Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

False negative PT_RELATIVE_PATH_TRAVERSAL for Java 11 and higher #2184

Closed
baloghadamsoftware opened this issue Sep 20, 2022 · 1 comment
Closed

Comments

@baloghadamsoftware
Copy link
Contributor

Given the following code (from bugIdeas.Ideas_2010_12_06.java):

    public void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
        response.setContentType("text/plain");
        PrintWriter out = response.getWriter();
        String path = request.getParameter("path");
        BufferedReader r = new BufferedReader(new FileReader("data/" + path));
        while (true) {
            String txt = r.readLine();
            if (txt == null)
                break;
            out.println(txt);
        }
        out.close();
        r.close();
    }

In Java 8 this translates to bytecode:

  public void doGet(javax.servlet.http.HttpServletRequest, javax.servlet.http.HttpServletResponse) throws javax.servlet.ServletException, java.io.IOException;
    Code:
       0: aload_2
       1: ldc           #7                  // String text/plain
       3: invokeinterface #9,  2            // InterfaceMethod javax/servlet/http/HttpServletResponse.setContentType:(Ljava/lang/String;)V
       8: aload_2
       9: invokeinterface #15,  1           // InterfaceMethod javax/servlet/http/HttpServletResponse.getWriter:()Ljava/io/PrintWriter;
      14: astore_3
      15: aload_1
      16: ldc           #19                 // String path
      18: invokeinterface #21,  2           // InterfaceMethod javax/servlet/http/HttpServletRequest.getParameter:(Ljava/lang/String;)Ljava/lang/String;
      23: astore        4
      25: new           #27                 // class java/io/BufferedReader
      28: dup
      29: new           #29                 // class java/io/FileReader
      32: dup
      33: new           #31                 // class java/lang/StringBuilder
      36: dup
      37: invokespecial #33                 // Method java/lang/StringBuilder."<init>":()V
      40: ldc           #34                 // String data/
      42: invokevirtual #36                 // Method java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
      45: aload         4
      47: invokevirtual #36                 // Method java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
      50: invokevirtual #40                 // Method java/lang/StringBuilder.toString:()Ljava/lang/String;
      53: invokespecial #44                 // Method java/io/FileReader."<init>":(Ljava/lang/String;)V
      56: invokespecial #46                 // Method java/io/BufferedReader."<init>":(Ljava/io/Reader;)V
      59: astore        5
      61: aload         5
      63: invokevirtual #49                 // Method java/io/BufferedReader.readLine:()Ljava/lang/String;
      66: astore        6
      68: aload         6
      70: ifnonnull     76
      73: goto          85
      76: aload_3
      77: aload         6
      79: invokevirtual #52                 // Method java/io/PrintWriter.println:(Ljava/lang/String;)V
      82: goto          61
      85: aload_3
      86: invokevirtual #57                 // Method java/io/PrintWriter.close:()V
      89: aload         5
      91: invokevirtual #60                 // Method java/io/BufferedReader.close:()V
      94: return

The bug is caught by the CrossSiteScripting detector.

However, in Java 11 the byte code is totally different:

public void doGet(javax.servlet.http.HttpServletRequest, javax.servlet.http.HttpServletResponse) throws javax.servlet.ServletException, java.io.IOException;
    Code:
       0: aload_2
       1: ldc           #7                  // String text/plain
       3: invokeinterface #9,  2            // InterfaceMethod javax/servlet/http/HttpServletResponse.setContentType:(Ljava/lang/String;)V
       8: aload_2
       9: invokeinterface #15,  1           // InterfaceMethod javax/servlet/http/HttpServletResponse.getWriter:()Ljava/io/PrintWriter;
      14: astore_3
      15: aload_1
      16: ldc           #19                 // String path
      18: invokeinterface #21,  2           // InterfaceMethod javax/servlet/http/HttpServletRequest.getParameter:(Ljava/lang/String;)Ljava/lang/String;
      23: astore        4
      25: new           #27                 // class java/io/BufferedReader
      28: dup
      29: new           #29                 // class java/io/FileReader
      32: dup
      33: aload         4
      35: invokedynamic #31,  0             // InvokeDynamic #0:makeConcatWithConstants:(Ljava/lang/String;)Ljava/lang/String;
      40: invokespecial #34                 // Method java/io/FileReader."<init>":(Ljava/lang/String;)V
      43: invokespecial #36                 // Method java/io/BufferedReader."<init>":(Ljava/io/Reader;)V
      46: astore        5
      48: aload         5
      50: invokevirtual #39                 // Method java/io/BufferedReader.readLine:()Ljava/lang/String;
      53: astore        6
      55: aload         6
      57: ifnonnull     63
      60: goto          72
      63: aload_3
      64: aload         6
      66: invokevirtual #43                 // Method java/io/PrintWriter.println:(Ljava/lang/String;)V
      69: goto          48
      72: aload_3
      73: invokevirtual #48                 // Method java/io/PrintWriter.close:()V
      76: aload         5
      78: invokevirtual #51                 // Method java/io/BufferedReader.close:()V
      81: return

Therefore, the bug is missed by the detector.

The reason for the issue is exactly the same as for #2182 and #2183, but in another detector.

baloghadamsoftware pushed a commit to baloghadamsoftware/spotbugs that referenced this issue Sep 27, 2022
…tring concatenation in Java 9 and above

Instead of using StringBuffer or StringBuilder internally, Java 11 and above uses a dynamic call to makeConcatWithConstants() to append strings. Previously, `OpcodeStackDetector` did not handle the taint propagation properly in case of this dyanamic call which led to false negative such as the one described in issue [spotbugs#2184](spotbugs#2184). This PR fixes such issues by adding code to `OpcodeStackDetector` to handle this case as well.
KengoTODA added a commit that referenced this issue Oct 11, 2022
…tring concatenation in Java 11 and above (#2195)

* Test for Issue 2184

* Fix OpcodeStack to handle propagation of taints properly in case of string concatenation in Java 9 and above

Instead of using StringBuffer or StringBuilder internally, Java 11 and above uses a dynamic call to makeConcatWithConstants() to append strings. Previously, `OpcodeStackDetector` did not handle the taint propagation properly in case of this dyanamic call which led to false negative such as the one described in issue [#2184](#2184). This PR fixes such issues by adding code to `OpcodeStackDetector` to handle this case as well.

* Refactored double negatives

Co-authored-by: Kengo TODA <skypencil@gmail.com>
@JuditKnoll
Copy link
Collaborator

It seems like this one is solved by #2195.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants