http: setEncoding error for incoming packets

[iSkore]

added wrapping on socket.setEncoding to not allow encoding changes
on incoming packets.

Because HTTP must be in US-ASCII, this function should either not
be allowed, or should throw a standard stackTrace error if invoked
on an incoming packet.

Currently, the process encounters a fatal v8 error and crashes.
error report detailed in: issue #18118

Fixes: #18118

Ref: #19344

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

• _http_server (specifically: connectionListenerInternal method -> socket variable)

[addaleax]

I don’t think ascii should be allowed either; that would still give the assertion failure from the linked issue, right?

[jasnell]

hmm... yeah, I think what we'd want is to just eliminate the ability to setEncoding on these sockets entirely.

[iSkore]

@jasnell: Yes, setting the encoding on incoming packets (for HTTP based transactions) should not be allowed. Setting the encoding on outgoing packets could be allowed (?) but I can't think of a practical use case. Nevertheless, any HTTP response should not allow other encodings.
Ref: RFC20
Ref: RFC2616

@addaleax: yup, the linked issue is a the result of setting the encoding on incoming packets transferred via HTTP. Incoming HTTP packets have to be ascii encoded already. Not sure why, but running setEncoding( 'ascii' ) results in the same failure. Redacting setEncoding is probably the best solution for the http/https subsystem.

[addaleax]

Not sure why, but running setEncoding( 'ascii' ) results in the same failure.

Because setEncoding('ascii') still makes the stream emit strings, not buffers, and the parser is expecting buffers -- that's all, I think.

[iSkore]

Ah, that makes since. I see that now in the http_parser.c file - this pull request is broken.
How would you approach solving this issue?

Because http extends net, should setEncoding not be inherited? As in "deleted" at the point where http inherits net? (seems hacky to me)

Or should it be removed from the net module as well?

Or perhaps just overriding the setEncoding method to throw an error if used?

socket.setEncoding = undefined;

Should throw a TypeError TypeError: socket.setEncoding is not a function

Ref: RFC20 suggests that all network transactions should be in ascii. But, perhaps there are considerations for other protocol's, maybe someone wants to make a "custom" protocol that sends utf8 or something like that.

[jasnell]

The method should be overridden to throw.

[jasnell]

Forcing the socket data to ASCII is definitely not the right thing to do. With http, for instance, while the protocol bits themselves are generally ascii, the payload data typically is not. Forcing the socket to always ascii would result in data corruption. The best thing here is to simply require the socket to use buffers and let the bits that handle the parsing for specific bits to deal with the encoding.

[iSkore]

@jasnell @addaleax per this discussion, I updated the setEncoding method to be undefined in the incoming packet handler connectionListenerInternal

[addaleax]

The method should be overridden to throw.

+1, I agree that’s the right thing to do here.

@iSkore could you also make sure that this interacts well with Connection: upgrade? It’s not obvious to me whether that case is affected, but you should be able to set .setEncoding() after a protocol switch.

[iSkore]

@addaleax - do you think this fix would be in the _http_server.js method or should I go into the parsing handlers in the _http_common.js? Still figuring out where things are, but I've narrowed it down to the onParserExecute method. I'm having an issue with detecting if it's an upgrade request before the execute function is invoked on line 377

So my first thought was to check if the headers are requesting an upgrade, if not, override the setEncoding method. But I can't see if it's an upgrade until the onParserExecuteCommon - but it breaks before the first line in that method is called because parser.consume(external); is running first.

Any pointers on what file(s) I should focus on to fix this?

[jasnell]

@iSkore ... rather than setting setEncoding to undefined, it should be something like...

1. first, create a new top level function to avoid creating a closure:

function socketSetEncoding() {
  throw new errors.Error('ERR_METHOD_NOT_IMPLEMENTED',
                         'setEncoding');
}

2. then....

socket.setEncoding = socketSetEncoding;

3. if/when the socket instance is ever released by the http implementation, the original setEncoding implementation should be restored, if possible.

[iSkore]

Upon further research, I'm seeing that any encoding changes should not be allowed.

RFC6455 Section 10.7

A common class of security problems arises when sending text data
using the wrong encoding. This protocol specifies that messages with
a Text data type (as opposed to Binary or other types) contain UTF-8-
encoded data. Although the length is still indicated and
applications implementing this protocol should use the length to
determine where the frame actually ends, sending data in an improper
encoding may still break assumptions that applications built on top
of this protocol may make, leading to anything from misinterpretation
of data to loss of data or potential security bugs.

I interpret this as a fairly generic statement (compared to other RFC literature) suggesting that it should not be allowed at all:

leading to anything from misinterpretation of data to loss of data or potential security bugs.

For the language as a whole, particularly the net module, I can see this being too strict - but for the scope of HTTP, it's a relevant concern.

[BridgeAR]

@iSkore would you be so kind and change the implementation accordingly to @jasnell s suggestion? And adding a test case that would normally result in a crash would be awesome.

As far as I see it, it should not matter if there is a upgrade is used or not but I did not look into that closely to be sure.

[iSkore]

@BridgeAR - Yup, the crash report is detailed in #18118
Should I add a test to the test/async-hooks/test-httpparser.response.js file?

[BridgeAR]

@iSkore this has nothing to do with async-hooks, so it should be in test/parallel/test-http*.js. I am not sure if there is a good file to add another test to but it is probably also fine to create a new one.

[iSkore]

added setEncoding override error and tests

[iSkore]

@BridgeAR - should the Net module documentation be edited in someway to say "this method can only be used for response encodings?

[BridgeAR]

@iSkore I do not have a strong opinion. But maybe someone else might say something about that.

[BridgeAR]

Ping @nodejs/http @nodejs/http2 PTAL