Make crypto functions accept buffers as arguments #2945

Closed
wants to merge 21 commits into
from

Projects

None yet

4 participants

This will someday fix #1393

I think with these last commits (up to f28be11) I've finished converting the arguments (inputs). I have also updated the docs and will start working on the return values (outputs) next.

By the way, I can rebase this if needed. Just say so.

Owner

Looks good in general but I would opt for a simple RAII class instead of doing manual memory management with RetrieveData(). It's something I've been wanting to do anyway so might as well do it now.

Good idea! But maybe even that isn't needed in case we ditch encoded strings altogether. What do you think, @bnoordhuis?

Owner

That's not an option. We deprecate things before removing them. People need a chance to update their code.

Sure. What I'm trying to say is that this should be deprecated and removed, since node seems to recommend buffers instead of encoded strings anyway. But this isn't the forum to discuss that.

I'll refactor the code to use a RAII class and then begin converting the function outputs.

If I have some time, I'll try to take a look at the rest of the core lib codebase (besides crypto) to check for other places that could use this improvement. That will help with deprecating encoded strings in the future.

Here alloc_buf was previously set to false, but I thought it needed to be true since buf will soon be complete_hex that was allocated in line 2525/2534, thus needing to be free'd.

But maybe the previous code was right and I may have introduced a possible buffer overflow caused by a double free.

I think that does it. Would someone please take a look?

I am not really sure about the changes to DecipherUpdate in 55fcf52. Please take a little more care when reviewing it (I've added a comment to the region that makes me unease).

Owner

Sorry, I haven't had time to review the PR yet.

No problem, @bnoordhuis. Take your time...

Is there added value against 63ff449?

isaacs commented Dec 30, 2012

This was already done on master.

@isaacs isaacs closed this Dec 30, 2012
@joaocgreis joaocgreis pushed a commit to janeasystems/node-v0.x-archive that referenced this pull request Sep 23, 2015
@rvagg rvagg 2015-09-22, Version 4.1.1 (Stable) Release
Notable changes

* buffer: Fixed a bug introduced in v4.1.0 where allocating a new
  zero-length buffer can result in the next allocation of a TypedArray
  in JavaScript not being zero-filled. In certain circumstances this
  could result in data leakage via reuse of memory space in
  TypedArrays, breaking the normally safe assumption that TypedArrays
  should be always zero-filled. (Trevor Norris) #2931.
* http: Guard against response-splitting of HTTP trailing headers
  added via response.addTrailers() by removing new-line ([\r\n])
  characters from values. Note that standard header values are already
  stripped of new-line characters. The expected security impact is low
  because trailing headers are rarely used. (Ben Noordhuis) #2945.
* npm: Upgrade to npm 2.14.4 from 2.14.3, see release notes for full
  details (Kat Marchán) #2958
  - Upgrades graceful-fs on multiple dependencies to no longer rely on
    monkey-patching fs
  - Fix npm link for pre-release / RC builds of Node
* v8: Update post-mortem metadata to allow post-mortem debugging tools
  to find and inspect:
  - JavaScript objects that use dictionary properties
    (Julien Gilli) #2959
  - ScopeInfo and thus closures (Julien Gilli) #2974

PR-URL: nodejs/node#2995
4a6f1fe
@richardlau richardlau pushed a commit to ibmruntimes/node that referenced this pull request Sep 23, 2015
@rvagg rvagg 2015-09-22, Version 4.1.1 (Stable) Release
Notable changes

* buffer: Fixed a bug introduced in v4.1.0 where allocating a new
  zero-length buffer can result in the next allocation of a TypedArray
  in JavaScript not being zero-filled. In certain circumstances this
  could result in data leakage via reuse of memory space in
  TypedArrays, breaking the normally safe assumption that TypedArrays
  should be always zero-filled. (Trevor Norris) #2931.
* http: Guard against response-splitting of HTTP trailing headers
  added via response.addTrailers() by removing new-line ([\r\n])
  characters from values. Note that standard header values are already
  stripped of new-line characters. The expected security impact is low
  because trailing headers are rarely used. (Ben Noordhuis) #2945.
* npm: Upgrade to npm 2.14.4 from 2.14.3, see release notes for full
  details (Kat Marchán) #2958
  - Upgrades graceful-fs on multiple dependencies to no longer rely on
    monkey-patching fs
  - Fix npm link for pre-release / RC builds of Node
* v8: Update post-mortem metadata to allow post-mortem debugging tools
  to find and inspect:
  - JavaScript objects that use dictionary properties
    (Julien Gilli) #2959
  - ScopeInfo and thus closures (Julien Gilli) #2974
ab55b45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment