9ee16daa47
* Update JSON schema to include STIG ASD 2023-06-08 mapping * Update rules to add STIG metadata mappings --------- Co-authored-by: Loris Sierra <loris.sierra@sonarsource.com>
212 lines
7.3 KiB
Plaintext
212 lines
7.3 KiB
Plaintext
== Why is this an issue?
|
||
|
||
The `processHttpRequest` method and methods called from it can be executed by multiple threads within the same servlet instance, and state changes to the instance caused by these methods are, therefore, not threadsafe.
|
||
|
||
This is due to the servlet container creating only one instance of each servlet (`javax.servlet.http.HttpServlet`) and attaching a dedicated thread to each incoming HTTP request.
|
||
The same problem exists for `org.apache.struts.action.Action` but with different methods.
|
||
|
||
To prevent unexpected behavior, avoiding mutable states in servlets is recommended.
|
||
Mutable instance fields should either be refactored into local variables or made immutable by declaring them `final`.
|
||
|
||
=== Exceptions
|
||
|
||
* Fields annotated with ``++@javax.inject.Inject++``, ``++@javax.ejb.EJB++``, ``++@org.springframework.beans.factory.annotation.Autowired++``, ``++@javax.annotation.Resource++``
|
||
* Fields initialized in ``++init()++`` or ``++init(ServletConfig config)++`` methods
|
||
|
||
== How to fix it
|
||
|
||
=== Code examples
|
||
|
||
==== Noncompliant code example
|
||
|
||
If the field is never modified, declare it `final`.
|
||
|
||
[source,java,diff-id=1,diff-type=noncompliant]
|
||
----
|
||
public class MyServlet extends HttpServlet {
|
||
String apiVersion = "0.9.1"; // Noncompliant, field changes are not thread-safe
|
||
}
|
||
----
|
||
|
||
==== Compliant solution
|
||
|
||
[source,java,diff-id=1,diff-type=compliant]
|
||
----
|
||
public class MyServlet extends HttpServlet {
|
||
final String apiVersion = "0.9.1"; // Compliant, field cannot be changed
|
||
}
|
||
----
|
||
|
||
==== Noncompliant code example
|
||
|
||
If a field is modified within instance methods, refactor it into a local variable.
|
||
That variable can be passed as an argument to other functions if needed.
|
||
|
||
[source,java,diff-id=2,diff-type=noncompliant]
|
||
----
|
||
public class MyServlet extends HttpServlet {
|
||
|
||
String userName; // Noncompliant, field changes are not thread-safe
|
||
|
||
@Override
|
||
public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException {
|
||
userName = req.getParameter("userName"); // Different threads may write concurrently to userName
|
||
resp.getOutputStream().print(getGreeting());
|
||
}
|
||
|
||
public String getGreeting() { // Unpredictable value in field userName
|
||
return "Hello "+userName+"!";
|
||
}
|
||
}
|
||
----
|
||
|
||
==== Compliant solution
|
||
|
||
[source,java,diff-id=2,diff-type=compliant]
|
||
----
|
||
public class MyServlet extends HttpServlet {
|
||
|
||
@Override
|
||
public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException {
|
||
String userName = req.getParameter("userName"); // Compliant, local variable instead instance field
|
||
resp.getOutputStream().print(getGreeting(userName));
|
||
}
|
||
|
||
public String getGreeting(String userName) { // Compliant, method argument instead instance field
|
||
return "Hello "+userName+"!";
|
||
}
|
||
}
|
||
----
|
||
|
||
==== Noncompliant code example
|
||
|
||
If you still prefer instance state over local variables, consider using `ThreadLocal` fields.
|
||
These fields provide a separate instance of their value for each thread.
|
||
|
||
[source,java,diff-id=3,diff-type=noncompliant]
|
||
----
|
||
public class MyServlet extends HttpServlet {
|
||
|
||
String userName; // Noncompliant, field changes are not thread-safe
|
||
|
||
@Override
|
||
public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException {
|
||
userName = req.getParameter("userName"); // Different threads may write concurrently to userName
|
||
resp.getOutputStream().print(getGreeting());
|
||
}
|
||
|
||
public String getGreeting() { // Unpredictable value in field userName
|
||
return "Hello "+userName+"!";
|
||
}
|
||
}
|
||
----
|
||
|
||
==== Compliant solution
|
||
|
||
[source,java,diff-id=3,diff-type=compliant]
|
||
----
|
||
public class MyServlet extends HttpServlet {
|
||
|
||
final ThreadLocal<String> userName = new ThreadLocal<>(); // Compliant, field itself does not change
|
||
|
||
@Override
|
||
public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException {
|
||
userName.set(req.getParameter("userName")); // Compliant, own value provided for every thread
|
||
resp.getOutputStream().print(getGreeting());
|
||
}
|
||
|
||
public String getGreeting() {
|
||
return "Hello "+userName.get()+"!"; // Compliant, own value provided for every thread
|
||
}
|
||
}
|
||
----
|
||
|
||
==== Noncompliant code example
|
||
|
||
If you have a use case that requires a shared instance state between threads, declare the corresponding fields as `static` to indicate your intention and
|
||
awareness that there is only one instance of the servlet.
|
||
However, the `static` modifier alone does not ensure thread safety.
|
||
Make sure also to take countermeasures against possible race conditions.
|
||
|
||
[source,java,diff-id=4,diff-type=noncompliant]
|
||
----
|
||
public class MyServlet extends HttpServlet {
|
||
|
||
public long timestampLastRequest; // Noncompliant, field changes are not thread-safe
|
||
|
||
@Override
|
||
public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException {
|
||
timestampLastRequest = System.currentTimeMillis();
|
||
resp.getOutputStream().print(timestampLastRequest); // Race condition
|
||
}
|
||
}
|
||
----
|
||
|
||
==== Compliant solution
|
||
|
||
[source,java,diff-id=4,diff-type=compliant]
|
||
----
|
||
public class MyServlet extends HttpServlet {
|
||
|
||
public static long timestampLastRequest; // Compliant, sharing state is our intention
|
||
|
||
@Override
|
||
public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException {
|
||
long timestamp;
|
||
synchronized (this) {
|
||
timestamp = timestampLastRequest; // No race condition, synchronized get & set
|
||
timestampLastRequest = System.currentTimeMillis();
|
||
}
|
||
resp.getOutputStream().print(timestamp);
|
||
}
|
||
}
|
||
----
|
||
|
||
== Resources
|
||
|
||
=== Articles & blog posts
|
||
|
||
* https://www.devinline.com/2013/08/how-to-make-thread-safe-servlet.html[Nikhil Ranjan: How to make thread safe servlet ?]
|
||
* https://objectcomputing.com/resources/publications/sett/april-2000-tips-for-creating-thread-safe-code-avoiding-race-conditions[Object Computing: Tips for creating thread-safe code]
|
||
|
||
=== Standards
|
||
|
||
* STIG Viewer - https://stigviewer.com/stig/application_security_and_development/2023-06-08/finding/V-222567[Application Security and Development: V-222567] - The application must not be vulnerable to race conditions.
|
||
|
||
|
||
ifdef::env-github,rspecator-view[]
|
||
|
||
'''
|
||
== Implementation Specification
|
||
(visible only on this page)
|
||
|
||
=== Message
|
||
|
||
Remove this misleading mutable servlet instance fields or make it "static" and/or "final"
|
||
|
||
'''
|
||
== Comments And Links
|
||
(visible only on this page)
|
||
|
||
=== is related to: S2223
|
||
|
||
=== on 25 Nov 2014, 11:00:13 Freddy Mallet wrote:
|
||
\[~ann.campbell.2] If you want I can take care to fully rewrite the rule in something like "Servlet should not have misleading non-static fields"
|
||
|
||
=== on 25 Nov 2014, 12:28:16 Ann Campbell wrote:
|
||
\[~freddy.mallet] the original requester was specific that the rule shouldn't be limited to just ``++Servlet++`` classes, but I'm happy to go along if you feel that would make a better rule.
|
||
|
||
|
||
BTW, he's also asking for an ignoreClasses parameter.
|
||
|
||
=== on 7 Mar 2019, 23:27:30 Victor Matskiv wrote:
|
||
The issue is not aligned with servlet semantics. Specifically:
|
||
|
||
|
||
A servlet can be legitimately initialized from ServletContext using ``++init(ServletContext)++`` method. This makes it impossible to qualify servlet fields as final.
|
||
|
||
|
||
Another suggestion to make servlet fields static introduces rather misleading semantics and contradicts the referenced document: \https://wiki.sei.cmu.edu/confluence/display/java/MSC11-J.+Do+not+let+session+information+leak+within+a+servlet
|
||
|
||
endif::env-github,rspecator-view[]
|