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

http: setEncoding error for incoming socket connections #19344

Closed
wants to merge 9 commits into from

Conversation

iSkore
Copy link
Contributor

@iSkore iSkore commented Mar 14, 2018

  • added override on socket.setEncoding to not allow encoding changes on incoming socket connections
  • added error ERR_HTTP_INCOMING_SOCKET_ENCODING
  • added tests and documentation

Original PR-URL: #18178 (new pull request made due to git issues)

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

Ref: #18178
Fixes: #18118

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)
  • _http_server
  • doc
  • errors
  • tests

@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. http Issues or PRs related to the http subsystem. labels Mar 14, 2018
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually something also defined in any of these?

RFC 7230, HTTP/1.1: Message Syntax and Routing
RFC 7231, HTTP/1.1: Semantics and Content
RFC 7232, HTTP/1.1: Conditional Requests
RFC 7233, HTTP/1.1: Range Requests
RFC 7234, HTTP/1.1: Caching
RFC 7235, HTTP/1.1: Authentication

@@ -728,6 +728,9 @@ E('ERR_HTTP2_STREAM_SELF_DEPENDENCY',
E('ERR_HTTP2_UNSUPPORTED_PROTOCOL', 'protocol "%s" is unsupported.', Error);
E('ERR_HTTP_HEADERS_SENT',
'Cannot %s headers after they are sent to the client', Error);
E('ERR_HTTP_INCOMING_SOCKET_ENCODING',
'Incoming socket encoding is not allowed (RFC 2616)',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please change this to e.g., Changing the incoming socket encoding is not possible (see RFC 2616)?.

### ERR_HTTP_INCOMING_SOCKET_ENCODING

An attempt was made to manipulate the encoding of an incoming request packet.
This is not allowed (RFC2616).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: This is forbidden by [RFC 2616](https://www.ietf.org/rfc/rfc2616.txt).

@iSkore
Copy link
Contributor Author

iSkore commented Mar 14, 2018

@BridgeAR yes, the use of US-ASCII is noted frequently throughout the RFC articles but the most important is:

A recipient MUST parse an HTTP message as a sequence of octets in an encoding that is a superset of US-ASCII [USASCII]. RFC7230 Section 3

Should I reference RFC7230 instead?

@iSkore
Copy link
Contributor Author

iSkore commented Mar 14, 2018

I do believe RFC7230 - RFC7240 is a collection of updates to RFC2616

@iSkore
Copy link
Contributor Author

iSkore commented Mar 29, 2018

@BridgeAR wanted to follow up with you. Will this PR be considered for the v10.0.0 semver-major release like #18178?

@jasnell jasnell added this to the 10.0.0 milestone Mar 29, 2018
@BridgeAR BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 29, 2018
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

### ERR_HTTP_INCOMING_SOCKET_ENCODING

An attempt was made to manipulate the encoding of an incoming request packet.
This is forbidden by [RFC7230 Section 3](https://tools.ietf.org/html/rfc7230#section-3)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this is longer than 80 characters and the linter is probably going to complain about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It did - I wasn't sure what to do because it was a URL and it appears to be the only one in the docs that is written this way. Still thought it was worth referencing the RFC because this is such a specific issue with a specific fix with no other options - so I left it
This looks better 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe what we generally do is put all link URLs at the end of the file, and then reference the label here. The same could be done here.

@@ -683,6 +685,10 @@ function onSocketPause() {
}
}

function socketSetEncoding() {
throw new ERR_HTTP_INCOMING_SOCKET_ENCODING('setEncoding');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no dynamic argument in this error, so there is no need to pass through any arguments.

@BridgeAR
Copy link
Member

@iSkore
Copy link
Contributor Author

iSkore commented Mar 29, 2018

Looks like node-test-commit-arm-fanned broke on zlib-binding on pi1-raspbian-wheezy OS - should that get a re-run or should I investigate?

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the code changes look good to me, the reason why setEncoding('utf8') doesn't work is not because HTTP always uses US-ASCII, but rather that Node.js C++ internals are not prepared to deal with strings rather than Buffers. In fact, the same crash would happen if one uses setEncoding('ascii').

As such, the error message and the documentation should be amended, to something to the sense of "changing it is not supported" rather than not allowed by the RFC.

### ERR_HTTP_INCOMING_SOCKET_ENCODING

An attempt was made to manipulate the encoding of an incoming request packet.
This is forbidden by [RFC7230 Section 3](https://tools.ietf.org/html/rfc7230#section-3)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe what we generally do is put all link URLs at the end of the file, and then reference the label here. The same could be done here.

@@ -383,6 +384,7 @@ function connectionListenerInternal(server, socket) {

// Override on to unconsume on `data`, `readable` listeners
socket.on = socketOnWrap;
socket.setEncoding = socketSetEncoding;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: either a comment or a new line here would be nice, since this line isn't related to

  // Override on to unconsume on `data`, `readable` listeners

Copy link
Contributor Author

@iSkore iSkore Mar 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, because I didn't see that done in errors.md(aside from internal page links) I tried to follow suit. I'll update that.

@BridgeAR
Copy link
Member

@TimothyGu
Copy link
Member

@apapirovski, @BridgeAR Go ahead!

@iSkore
Copy link
Contributor Author

iSkore commented Apr 19, 2018

Firstly, new TSC member @TimothyGu, congrats!!

Second, with the release date coming up, I'd like to get these test passing. By the looks of it, the build errors are due to linting problems. When I run make lint and ./configure && make -j4 test I don't get any errors.

More make lint I get:

Running JS linter...
make[1]: Nothing to be done for `lint-cpp'.
Running C++ linter on addon docs...
Total errors found: 0
make[1]: Nothing to be done for `lint-md'.

And for ./configure && make -j4 test I get:

[  PASSED  ] 73 tests.
.....
[03:18|% 100|+ 2225|-   0]: Done  

@BridgeAR, @TimothyGu, @apapirovski any suggestions?

@@ -383,6 +384,8 @@ function connectionListenerInternal(server, socket) {

// Override on to unconsume on `data`, `readable` listeners
socket.on = socketOnWrap;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this line has trailing space (should be 0 length).

const server = http.createServer().listen(0, connectToServer);

server.on('connection', (socket) => {
common.expectsError(() => socket.setEncoding(''),
Copy link
Member

@apapirovski apapirovski Apr 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you might still be testing the old setEncoding here. I think you might be better off just using normal request callback and then using req.socket.setEncoding.

But maybe I'm missing something else that's off about this... not sure. It is throwing an AssertionError though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That req part isn't an option in the 'connection' event. Are you saying to test with an incoming request after the 'connection'? This wouldn't throw an error because the request is parsed by then.

For example:

http.createServer( ( req, res ) => {
    req.socket.setEncoding( 'utf8' );
    res.end( 'OK' );
} )

is ok to do. It's only "pre-parse" attempts to manipulate the incoming buffer

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you can ignore me. It's complaining about the fact that you're providing an argument to the error, as mentioned above by @BridgeAR.

But if you're saying it's ok then this PR isn't quite valid because calling that req.socket.setEncoding('utf8') will still throw as things exist currently.

Copy link
Member

@apapirovski apapirovski Apr 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But also this isn't strictly correct. It's definitely not OK to call req.socket.setEncoding until after the end event. (Basically: it's never OK within the lifetime of its usefulness so we might as well just throw unconditionally.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Do you know any history behind setEncoding and how it even came about?

Right now I override setEncoding in the connectionListenerInternal method. This causes a fatal error as detailed in #18118. Are you recommending we remove it completely from the HTTP module?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, we got our wires crossed. I'm just referencing this bit...

Are you saying to test with an incoming request after the 'connection'? This wouldn't throw an error because the request is parsed by then.

It would throw throughout the requests lifetime. Sockets that are used for http should never have their encoding changed. I'm just saying that this is also correct behaviour.

To simplify: I'm fine with this PR as is except for the linting issue and the one pointed out by @BridgeAR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment from @addaleax on the original PR suggests there are other uses of setEncoding when possibly responding to a protocol change request or connection upgrade requests.

There's a slight chance it could be necessary when using abstract protocols such as Internet Printing Protocol/1.0 or some other media protocols. Although, as I discussed in that PR, this should not be allowed in the scope of HTTP. HTTP in, HTTP out.

So this would require moving socket.setEncoding = socketSetEncoding; to somewhere in the httpSocketSetup method or something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iSkore Thanks for the link. I might need to think about this more. I think that particular use-case would be addressed by resetting the socket setEncoding back within the the upgrade code path but there might be others that are not addressed. This might also affect how modules like ws operate. Let me do some research or feel free to do it yourself — but it seems like this PR will need to be updated to account for at least upgrade, if not more.

@jasnell
Copy link
Member

jasnell commented Apr 19, 2018

Just fyi... it's too late to get this in to 10.0.0. Unless they're super critical, there will be no more majors pulled in.

@jasnell jasnell modified the milestones: 10.0.0, 11.0.0 Apr 19, 2018
@apapirovski apapirovski removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 19, 2018
@iSkore
Copy link
Contributor Author

iSkore commented Apr 19, 2018

@jasnell - gotcha.

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just marking with red X so no one accidentally lands this. The use-case of upgrade or sockets that are both http and, say, WS, needs to be addressed.

@iSkore
Copy link
Contributor Author

iSkore commented Apr 20, 2018

@apapirovski no problem.

Some more information for discussion.

My interpretation of RFC 7230 Section 6.7 is that regardless of the upgrade request, the response must still be in US-ASCII.

Example:

HTTP/1.1 101 Switching Protocols
Connection: upgrade
Upgrade: HTTP/2.0

Although, it also states:

A server MUST NOT switch protocols unless the received message semantics can be honored by the new protocol;

That protocol could have a different encoding... but RFC20 ASCII format for Network Interchange implores those protocols be in ASCII.

As for handling WebSocket Protocol RFC 6455
RFC 6455 Section 4.2.1 bullet point 3:

An |Upgrade| header field containing the value "websocket", treated as an ASCII case-insensitive value.

So in the case of an upgrade to WebSocket Protocol, the HTTP response and WS interchange should both be ASCII.

With all that TL;DR information said - this PR fixes a fatal issue detailed in #18118. Should the fix to the socket.on( 'connection' ) event be pushed?

@apapirovski apapirovski self-assigned this May 4, 2018
@ryzokuken
Copy link
Contributor

ping @iSkore @apapirovski what's the status on this?

@ryzokuken
Copy link
Contributor

ping @iSkore

Are you planning to move this one forward, or can this be closed? It has been abandoned for months now.

@apapirovski
Copy link
Member

@ryzokuken this is held up on me. We need to solve the problem of sockets that are upgraded or otherwise detached from http.

@iSkore
Copy link
Contributor Author

iSkore commented Aug 18, 2018

@apapirovski, following up on this.

Should I add in a check like "if socket is getting upgraded, allow setEncoding, otherwise throw ERR_HTTP_INCOMING_SOCKET_ENCODING"

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Oct 16, 2018
@jasnell
Copy link
Member

jasnell commented Oct 17, 2018

This is currently on the 11.0.0 milestone, but as a semver-major, even if it lands right now, it's going to miss the release. Removing it from the 11.0.0 milestone.

@jasnell jasnell removed this from the 11.0.0 milestone Oct 17, 2018
@Trott
Copy link
Member

Trott commented Nov 10, 2018

/ping @apapirovski

@fhinkel
Copy link
Member

fhinkel commented Oct 26, 2019

Closing this due to inactivity. Please reopen if needed.

@fhinkel fhinkel closed this Oct 26, 2019
iSkore added a commit to iSkore/node that referenced this pull request May 14, 2020
applied updates from previous pull-requests to disallow
socket.setEncoding before a http connection is parsed.
wrapped socket.setEncoding to throw an error.
previously resulted in a fatal error

Fixes: nodejs#18118
Ref: nodejs#18178
Ref: nodejs#19344
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 23, 2020
Applied updates from previous pull-requests to disallow
socket.setEncoding before a http connection is parsed.
Wrapped `socket.setEncoding` to throw an error.
This previously resulted in a fatal error.

PR-URL: nodejs#33405
Fixes: nodejs#18118
Refs: nodejs#18178
Refs: nodejs#19344
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. http Issues or PRs related to the http subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fatal error when attempting to (wrongly) set incoming packet encoding
10 participants