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

http2: add aligned padding strategy #17938

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jan 1, 2018

Add a padding strategy option that makes a best attempt to ensure
that total frame length for DATA and HEADERS frames are aligned
on multiples of 8-bytes.

Ping @nodejs/http2

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

http2

Add a padding strategy option that makes a best attempt to ensure
that total frame length for DATA and HEADERS frames are aligned
on multiples of 8-bytes.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 1, 2018
@jasnell
Copy link
Member Author

jasnell commented Jan 1, 2018

Ping @addaleax ... for some reason I'm not able to open a new issue in the repo... but I can comment on issues and I can add PRs... very strange... In any case, I came across an issue you may want to look at:


@addaleax ... heads up... when using the ../common/duplexpair' with http2, and an error is thrown within a data event handler on the serverSide` half, we get an Abort...

That is, modifying test-http2-generic-streams like so (see the serverSide.on('data'... addition).

'use strict';
const common = require('../common');
if (!common.hasCrypto)
  common.skip('missing crypto');
const assert = require('assert');
const http2 = require('http2');
const makeDuplexPair = require('../common/duplexpair');

{
  const testData = '<h1>Hello World</h1>';
  const server = http2.createServer();
  server.on('stream', common.mustCall((stream, headers) => {
    stream.respond({
      'content-type': 'text/html',
      ':status': 200
    });
    stream.end(testData);
  }));

  const { clientSide, serverSide } = makeDuplexPair();
  server.emit('connection', serverSide);

  serverSide.on('data', () => {
    throw 'something';
  });

  const client = http2.connect('http://localhost:80', {
    createConnection: common.mustCall(() => clientSide)
  });

  const req = client.request({ ':path': '/' });

  req.on('response', common.mustCall((headers) => {
    assert.strictEqual(headers[':status'], 200);
  }));

  req.setEncoding('utf8');
  // Note: This is checking that this small amount of data is passed through in
  // a single chunk, which is unusual for our test suite but seems like a
  // reasonable assumption here.
  req.on('data', common.mustCall((data) => {
    assert.strictEqual(data, testData);
  }));
  req.on('end', common.mustCall(() => {
    clientSide.destroy();
    clientSide.end();
  }));
  req.end();
}

results in:

$ ./node test/parallel/test-http2-generic-streams.js
(node:27714) ExperimentalWarning: The http2 module is an experimental API.
FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal.
 1: node::Abort() [./node]
 2: 0x55d8e467592e [./node]
 3: v8::Utils::ReportApiFailure(char const*, char const*) [./node]
 4: node::JSStream::DoWrite(node::WriteWrap*, uv_buf_t*, unsigned long, uv_stream_s*) [./node]
 5: node::http2::Http2Session::SendPendingData() [./node]
 6: node::http2::Http2Session::OnStreamReadImpl(long, uv_buf_t const*, uv_handle_type, void*) [./node]
 7: node::JSStream::ReadBuffer(v8::FunctionCallbackInfo<v8::Value> const&) [./node]
 8: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [./node]
 9: 0x55d8e3ed6ba3 [./node]
10: v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) [./node]
11: 0x76c8100465d
Aborted (core dumped)

@addaleax
Copy link
Member

addaleax commented Jan 1, 2018

@jasnell I can’t open issues either. :/ Thanks for the pointer though!

* `http2.constants.PADDING_STRATEGY_ALIGNED` - Will *attempt* to apply
enough padding to ensure that the total frame length, including the
9-byte header, is a multiple of 8. For each frame, however, there is a
maxmimum allowed number of padding bytes that is determined by current
Copy link
Contributor

Choose a reason for hiding this comment

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

A nit: maxmimum -> maximum.

@jasnell jasnell added the http2 Issues or PRs related to the http2 subsystem. label Jan 2, 2018
@mcollina
Copy link
Member

mcollina commented Jan 2, 2018

when it is better to use one strategy or another? In other terms, should this be the default?

@jasnell
Copy link
Member Author

jasnell commented Jan 2, 2018

That's still unclear and needs to be benchmarked to determine the best strategy.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@mcollina
Copy link
Member

mcollina commented Jan 2, 2018

@jasnell after this is landed, can you open up a new issue for picking the best strategy?

@jasnell
Copy link
Member Author

jasnell commented Jan 2, 2018

Yep, definitely. The "best" is going to depend on a number of factors and may be difficult to nail down. Padding is supposed to be a security mechanism (see https://tools.ietf.org/html/rfc7540#section-10.7) but it's use really depends on a number of very data-specific and use-case-specific factors.

For instance, one reason why the aligned strategy might not make the best default is this particular passage in the RFC:

   Use of padding can result in less protection than might seem
   immediately obvious.  At best, padding only makes it more difficult
   for an attacker to infer length information by increasing the number
   of frames an attacker has to observe.  Incorrectly implemented
   padding schemes can be easily defeated.  In particular, randomized
   padding with a predictable distribution provides very little
   protection; similarly, padding payloads to a fixed size exposes
   information as payload sizes cross the fixed-sized boundary, which
   could be possible if an attacker can control plaintext.

In other words, aligned padding might may buffer allocation easier, but it also makes frame sizes more predictable.

@jasnell
Copy link
Member Author

jasnell commented Jan 3, 2018

@jasnell
Copy link
Member Author

jasnell commented Jan 3, 2018

CI is good

jasnell added a commit that referenced this pull request Jan 3, 2018
Add a padding strategy option that makes a best attempt to ensure
that total frame length for DATA and HEADERS frames are aligned
on multiples of 8-bytes.

PR-URL: #17938
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Jan 3, 2018

Landed in efdadcc

@jasnell jasnell closed this Jan 3, 2018
addaleax added a commit to addaleax/node that referenced this pull request Jan 7, 2018
Since these are executing JS code, and in particular parts of that
code may be provided by userland, handle such exceptions in C++.

Ref: nodejs#17938 (comment)
@addaleax addaleax mentioned this pull request Jan 7, 2018
3 tasks
MylesBorins pushed a commit to jasnell/node that referenced this pull request Jan 9, 2018
Add a padding strategy option that makes a best attempt to ensure
that total frame length for DATA and HEADERS frames are aligned
on multiples of 8-bytes.

PR-URL: nodejs#17938
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
Add a padding strategy option that makes a best attempt to ensure
that total frame length for DATA and HEADERS frames are aligned
on multiples of 8-bytes.

Backport-PR-URL: #18050
PR-URL: #17938
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
MylesBorins pushed a commit that referenced this pull request Jan 10, 2018
Add a padding strategy option that makes a best attempt to ensure
that total frame length for DATA and HEADERS frames are aligned
on multiples of 8-bytes.

Backport-PR-URL: #18050
PR-URL: #17938
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
addaleax added a commit that referenced this pull request Jan 14, 2018
Since these are executing JS code, and in particular parts of that
code may be provided by userland, handle such exceptions in C++.

Refs: #17938 (comment)
PR-URL: #18028
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
@ChALkeR ChALkeR added the security Issues and PRs related to security. label Jan 28, 2018
evanlucas pushed a commit that referenced this pull request Jan 30, 2018
Since these are executing JS code, and in particular parts of that
code may be provided by userland, handle such exceptions in C++.

Refs: #17938 (comment)
PR-URL: #18028
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
Add a padding strategy option that makes a best attempt to ensure
that total frame length for DATA and HEADERS frames are aligned
on multiples of 8-bytes.

Backport-PR-URL: nodejs#18050
PR-URL: nodejs#17938
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Add a padding strategy option that makes a best attempt to ensure
that total frame length for DATA and HEADERS frames are aligned
on multiples of 8-bytes.

Backport-PR-URL: #18050
Backport-PR-URL: #20456
PR-URL: #17938
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
MylesBorins pushed a commit that referenced this pull request May 22, 2018
Since these are executing JS code, and in particular parts of that
code may be provided by userland, handle such exceptions in C++.

Refs: #17938 (comment)
PR-URL: #18028
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
Since these are executing JS code, and in particular parts of that
code may be provided by userland, handle such exceptions in C++.

Refs: #17938 (comment)
PR-URL: #18028
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
Since these are executing JS code, and in particular parts of that
code may be provided by userland, handle such exceptions in C++.

Refs: #17938 (comment)
PR-URL: #18028
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. security Issues and PRs related to security.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants