http: setEncoding error for incoming socket connections #19344

iSkore · 2018-03-14T11:29:46Z

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

BridgeAR

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

BridgeAR · 2018-03-14T14:38:52Z

lib/internal/errors.js

@@ -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)',


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

BridgeAR · 2018-03-14T14:45:49Z

doc/api/errors.md

+### ERR_HTTP_INCOMING_SOCKET_ENCODING
+
+An attempt was made to manipulate the encoding of an incoming request packet.
+This is not allowed (RFC2616).


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

iSkore · 2018-03-14T19:08:31Z

@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 · 2018-03-14T19:11:09Z

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

iSkore · 2018-03-29T10:41:44Z

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

BridgeAR

LGTM

BridgeAR · 2018-03-29T12:08:26Z

doc/api/errors.md

+### 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)


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

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 👍

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.

BridgeAR · 2018-03-29T12:09:19Z

lib/_http_server.js

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

+function socketSetEncoding() {
+  throw new ERR_HTTP_INCOMING_SOCKET_ENCODING('setEncoding');


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

BridgeAR · 2018-03-29T12:09:55Z

CI https://ci.nodejs.org/job/node-test-pull-request/13923/

iSkore · 2018-03-29T15:36:54Z

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?

TimothyGu

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.

TimothyGu · 2018-03-29T17:54:02Z

doc/api/errors.md

+### 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)


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.

TimothyGu · 2018-03-29T17:54:47Z

lib/_http_server.js

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

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


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

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 · 2018-04-16T03:24:18Z

CI https://ci.nodejs.org/job/node-test-pull-request/14321/

TimothyGu · 2018-04-16T12:47:29Z

@apapirovski, @BridgeAR Go ahead!

iSkore · 2018-04-19T13:17:09Z

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?

apapirovski · 2018-04-19T14:40:49Z

lib/_http_server.js

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

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


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

apapirovski · 2018-04-19T14:46:18Z

test/parallel/test-http-socket-encoding-error.js

+const server = http.createServer().listen(0, connectToServer);
+
+server.on('connection', (socket) => {
+  common.expectsError(() => socket.setEncoding(''),


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.

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

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.

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.)

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?

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.

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?

@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 · 2018-04-19T15:18:10Z

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.

iSkore · 2018-04-19T16:03:39Z

@jasnell - gotcha.

PR-URL: nodejs#19344

apapirovski

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 · 2018-04-20T02:43:01Z

@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?

ryzokuken · 2018-06-07T19:14:36Z

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

ryzokuken · 2018-06-14T11:18:18Z

ping @iSkore

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

apapirovski · 2018-06-14T14:05:00Z

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

iSkore · 2018-08-18T14:23:13Z

@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 · 2018-10-17T20:08:28Z

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.

Trott · 2018-11-10T23:46:38Z

/ping @apapirovski

fhinkel · 2019-10-26T23:20:45Z

Closing this due to inactivity. Please reopen if needed.

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

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>

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

iSkore mentioned this pull request Mar 14, 2018

http: setEncoding error for incoming packets #18178

Closed

2 tasks

BridgeAR reviewed Mar 14, 2018

View reviewed changes

iSkore force-pushed the http/issue_18118 branch from 5ccd427 to a1e62c5 Compare March 14, 2018 19:24

jasnell added this to the 10.0.0 milestone Mar 29, 2018

iSkore force-pushed the http/issue_18118 branch from 772e6b0 to 63a72c6 Compare March 29, 2018 11:25

BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 29, 2018

BridgeAR requested review from mcollina, jasnell, addaleax, joyeecheung, a team, apapirovski and thefourtheye March 29, 2018 12:07

BridgeAR approved these changes Mar 29, 2018

View reviewed changes

BridgeAR reviewed Mar 29, 2018

View reviewed changes

TimothyGu reviewed Mar 29, 2018

View reviewed changes

iSkore force-pushed the http/issue_18118 branch from 63a72c6 to 52684df Compare March 29, 2018 21:35

apapirovski reviewed Apr 19, 2018

View reviewed changes

jasnell modified the milestones: 10.0.0, 11.0.0 Apr 19, 2018

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

http: minor change per review request

cd2c3ce

PR-URL: nodejs#19344

apapirovski requested changes Apr 19, 2018

View reviewed changes

apapirovski self-assigned this May 4, 2018

Merge branch 'master' into http/issue_18118

a8c605e

jasnell added the stalled Issues and PRs that are stalled. label Oct 16, 2018

jasnell removed this from the 11.0.0 milestone Oct 17, 2018

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:52

fhinkel closed this Oct 26, 2019

iSkore mentioned this pull request May 14, 2020

docs, errors, http, tests: fixed socket.setEncoding fatal error #33405

Closed

4 tasks

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

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

http: setEncoding error for incoming socket connections #19344

http: setEncoding error for incoming socket connections #19344

Conversation

iSkore commented Mar 14, 2018 • edited Loading

Checklist

Affected core subsystem(s)

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSkore commented Mar 14, 2018

iSkore commented Mar 14, 2018

iSkore commented Mar 29, 2018 • edited Loading

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Mar 29, 2018

iSkore commented Mar 29, 2018

TimothyGu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSkore Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

BridgeAR commented Apr 16, 2018

TimothyGu commented Apr 16, 2018

iSkore commented Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

apapirovski Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Apr 19, 2018

iSkore commented Apr 19, 2018

apapirovski left a comment

Choose a reason for hiding this comment

iSkore commented Apr 20, 2018

ryzokuken commented Jun 7, 2018

ryzokuken commented Jun 14, 2018

apapirovski commented Jun 14, 2018

iSkore commented Aug 18, 2018

jasnell commented Oct 17, 2018

Trott commented Nov 10, 2018

fhinkel commented Oct 26, 2019

iSkore commented Mar 14, 2018 •

edited

Loading

iSkore commented Mar 29, 2018 •

edited

Loading

TimothyGu left a comment •

edited

Loading

iSkore Mar 29, 2018 •

edited

Loading

iSkore commented Apr 19, 2018 •

edited

Loading

apapirovski Apr 19, 2018 •

edited

Loading

apapirovski Apr 19, 2018 •

edited

Loading