Jetty
  1. Jetty
  2. JETTY-376

Http Set Response method adds an extra underscore while sending the response

    Details

    • Type: Bug Bug
    • Status: Resolved Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: HTTP
    • Labels:
      None
    • Number of attachments :
      0

      Description

      I need to send some custom error messages to the client using Jetty. I saw in the code that setResponse message adds a underscore for each blank space entered in the custom message.
      Is there any workaround for this or can it betaken care of as I don't want my custom messages to be modified by library in anyway as they are parsed by the client.

      Code snippet from AbstractGenerator.java.

      public void setResponse(int status, String reason)
      {
      if (_state Unable to render embedded object: File (= STATE_HEADER) throw new IllegalStateException("STATE) not found.=START");

      _status = status;
      if (reason!=null)
      {
      int len=reason.length();
      if (len>_headerBufferSize/2)
      len=_headerBufferSize/2;
      _reason=new ByteArrayBuffer(len);
      for (int i=0;i<len;i++)

      { char ch = reason.charAt(i); if (Character.isWhitespace(ch)) _reason.put((byte)'_'); else if (Character.isJavaIdentifierPart(ch)) _reason.put((byte)ch); }

      }
      }

        Activity

        Hide
        Greg Wilkins added a comment -

        I am really loath to change this code.

        allowing arbitrary text into the reason string can open up a server to be exploited if any text from "the bad guys" is included in the
        reason string.

        Are you sure you can't send the information as a header?

        Show
        Greg Wilkins added a comment - I am really loath to change this code. allowing arbitrary text into the reason string can open up a server to be exploited if any text from "the bad guys" is included in the reason string. Are you sure you can't send the information as a header?
        Hide
        Pankaj Arora added a comment -

        Yes this is a really important requirement for us. The text send in reason string is used to provide information to posting clients in our use case.

        Show
        Pankaj Arora added a comment - Yes this is a really important requirement for us. The text send in reason string is used to provide information to posting clients in our use case.
        Hide
        Pankaj Arora added a comment -

        To make it more clear, its already allowed to change the response string. I am able to send whatever I want in the resposne string. Only problem is that all the sapces are replced with underscores, due to this code:

        for (int i=0;i<len;i++)

        { char ch = reason.charAt(i); if (Character.isWhitespace(ch)) _reason.put((byte)'_'); else if (Character.isJavaIdentifierPart(ch)) _reason.put((byte)ch); }

        }
        I don't want to modify my string. There is no reason to do that as per my understanding.

        Show
        Pankaj Arora added a comment - To make it more clear, its already allowed to change the response string. I am able to send whatever I want in the resposne string. Only problem is that all the sapces are replced with underscores, due to this code: for (int i=0;i<len;i++) { char ch = reason.charAt(i); if (Character.isWhitespace(ch)) _reason.put((byte)'_'); else if (Character.isJavaIdentifierPart(ch)) _reason.put((byte)ch); } } I don't want to modify my string. There is no reason to do that as per my understanding.
        Hide
        Greg Wilkins added a comment -

        the reason for this, is that there is plenty of code around that does something like:

        catch(Excpetion e)

        { response.sendError(500,e.toString()); }

        Now if the exception is something like UnknownUserExcpetion(String username)
        and an attacker provides a username that is

        xxx\r\n\r\nHTTP/1.0 200 OK\r\nContent-Length\r\n\r\nAnyContentTheyLike

        Then an attacker can inject an arbitrary content to be returned for the next request on
        the persistent connection.

        This is a real attack vector and Jetty has thus encoded reason strings since jetty 3
        and the servlet spec has deprecated the method that allows a reason to be set.

        But the RFC excludes on CR and LF, so we are being a bit overzealous. So I will look
        at changing this in the next release.

        Show
        Greg Wilkins added a comment - the reason for this, is that there is plenty of code around that does something like: catch(Excpetion e) { response.sendError(500,e.toString()); } Now if the exception is something like UnknownUserExcpetion(String username) and an attacker provides a username that is xxx\r\n\r\nHTTP/1.0 200 OK\r\nContent-Length\r\n\r\nAnyContentTheyLike Then an attacker can inject an arbitrary content to be returned for the next request on the persistent connection. This is a real attack vector and Jetty has thus encoded reason strings since jetty 3 and the servlet spec has deprecated the method that allows a reason to be set. But the RFC excludes on CR and LF, so we are being a bit overzealous. So I will look at changing this in the next release.
        Hide
        Greg Wilkins added a comment -

        spaces now allowed in svn

        Show
        Greg Wilkins added a comment - spaces now allowed in svn
        Hide
        Pankaj Arora added a comment -

        There is still some problem left with the fix :

        if (ch==' ' || Character.isJavaIdentifierPart(ch))
        _reason.put((byte)ch);

        This doesn't let me set any special characters like *,? etc to pass through. As Java doc states :
        isJavaIdentifierPart

        public static boolean isJavaIdentifierPart(char ch)

        Determines if the specified character may be part of a Java identifier as other than the first character.

        A character may be part of a Java identifier if any of the following are true:

        • it is a letter
        • it is a currency symbol (such as '$')
        • it is a connecting punctuation character (such as '_')
        • it is a digit
        • it is a numeric letter (such as a Roman numeral character)
        • it is a combining mark
        • it is a non-spacing mark
        • isIdentifierIgnorable returns true for the character

        Note: This method cannot handle supplementary characters. To support all Unicode characters, including supplementary characters, use the isJavaIdentifierPart(int) method.

        I don't see any reason for ignoring other special characters. Please do tell me if I am wrong , else this should be fixed and all the characters should be let in the send response.
        As already stated in the bug, this is very important for our use case to let the response string not be tampered with as our clients parse them for specific logic.

        Show
        Pankaj Arora added a comment - There is still some problem left with the fix : if (ch==' ' || Character.isJavaIdentifierPart(ch)) _reason.put((byte)ch); This doesn't let me set any special characters like *,? etc to pass through. As Java doc states : isJavaIdentifierPart public static boolean isJavaIdentifierPart(char ch) Determines if the specified character may be part of a Java identifier as other than the first character. A character may be part of a Java identifier if any of the following are true: it is a letter it is a currency symbol (such as '$') it is a connecting punctuation character (such as '_') it is a digit it is a numeric letter (such as a Roman numeral character) it is a combining mark it is a non-spacing mark isIdentifierIgnorable returns true for the character Note: This method cannot handle supplementary characters. To support all Unicode characters, including supplementary characters, use the isJavaIdentifierPart(int) method. I don't see any reason for ignoring other special characters. Please do tell me if I am wrong , else this should be fixed and all the characters should be let in the send response. As already stated in the bug, this is very important for our use case to let the response string not be tampered with as our clients parse them for specific logic.
        Hide
        Greg Wilkins added a comment -

        Resolved. I am now explicitly looking for CR or LF.

        Show
        Greg Wilkins added a comment - Resolved. I am now explicitly looking for CR or LF.

          People

          • Assignee:
            Greg Wilkins
            Reporter:
            Pankaj Arora
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: