Invalid read in node::base64_decode<char>(char*, unsigned long, char const*, unsigned long)

[kzc]

Found this while valgrinding the worker threads implementation:

==32704== Invalid read of size 1
==32704==    at 0xD1E319: unsigned long node::base64_decode<char>(char*, unsigned long, char const*, unsigned long)
==32704==    by 0xD1E8DD: node::StringBytes::Write(v8::Isolate*, char*, unsigned long, v8::Handle<v8::Value>, node::encoding, int*)
==32704==    by 0xD3AF14: node::crypto::Hash::HashUpdate(v8::FunctionCallbackInfo<v8::Value> const&)
==32704==    by 0x853B31: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&))
==32704==    by 0x87AD4A: v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*)
==32704==  Address 0x20e21598 is 0 bytes after a block of size 5,592,408 alloc'd
==32704==    at 0x4C2B800: operator new[](unsigned long)
==32704==    by 0xD1D7EA: node::StringBytes::Encode(v8::Isolate*, char const*, unsigned long, node::encoding)
==32704==    by 0xCF6E06: node::Buffer::Base64Slice(v8::FunctionCallbackInfo<v8::Value> const&)
==32704==    by 0x853B31: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&))
==32704==    by 0x87AD4A: v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*)

Likely fix:

--- a/src/string_bytes.cc
+++ b/src/string_bytes.cc
@@ -167,5 +167,5 @@ size_t base64_decode(char* buf,
     int remaining = srcEnd - src;

-    while (unbase64(*src) < 0 && src < srcEnd)
+    while (src < srcEnd && unbase64(*src) < 0)
       src++, remaining--;
     if (remaining == 0 || *src == '=')
@@ -173,5 +173,5 @@ size_t base64_decode(char* buf,
     a = unbase64(*src++);

-    while (unbase64(*src) < 0 && src < srcEnd)
+    while (src < srcEnd && unbase64(*src) < 0)
       src++, remaining--;
     if (remaining <= 1 || *src == '=')
@@ -183,5 +183,5 @@ size_t base64_decode(char* buf,
       break;

-    while (unbase64(*src) < 0 && src < srcEnd)
+    while (src < srcEnd && unbase64(*src) < 0)
       src++, remaining--;
     if (remaining <= 2 || *src == '=')
@@ -193,5 +193,5 @@ size_t base64_decode(char* buf,
       break;

-    while (unbase64(*src) < 0 && src < srcEnd)
+    while (src < srcEnd && unbase64(*src) < 0)
       src++, remaining--;
     if (remaining <= 3 || *src == '=')

[Fishrock123]

@kzc if this only happens on the workers PR you need to report it to that thread. :)

#2133

[kzc]

It's a universal problem independent of workers. Examine the code and you'll see that it was incorrect.

[targos]

@kzc do you have an example of code that shows this issue?

[kzc]

base64_decode() always reads a byte beyond the input length specified.

int main() {
  char* b64 = new char[4];
  b64[0] = 'Q';
  b64[1] = 'Q';
  b64[2] = '=';
  b64[3] = '=';
  char out[200];
  int len = base64_decode(out, sizeof(out), b64, 4);
  delete [] b64;
  printf("%.*s\n", len, out);
  return 0;
}

[brendanashworth]

@kzc a pull request to fix this would be gladly welcomed! :)

[thefourtheye]

@kzc Can this be reproduced with just JS code? I tried

console.log(new Buffer('QQ==', 'base64').toString());

it prints A, as expected.

[brendanashworth]

@thefourtheye did you valgrind the node binary while running that? A memory access into unowned territory wouldn't crash the process afaik, it's just "bad". (note I can't get valgrind working to confirm)

[reqshark]

Are you sure it's not just null terminating the string, that's what ur supposed to do in C?

[kzc]

Can this be reproduced with just JS code?

On the worker threads branch you can reproduce it with:

valgrind ./iojs --experimental-workers test/workers/test-crypto.js

But again I want to stress that the issue is not unique to that branch. Apparently with a call to some crypto hash update method it is possible to create such a case - see the valgrind stack traces at the top of this ticket. Unfortunately I don't have the time to isolate the specific sequence of JS instructions that lead to the valgrind error.

Even if reading an extra byte beyond the input byte range seems harmless in most cases, it does waste a few CPU cycles on computing an extra unbase64(*src) for each and every base64_decode() call for no good reason - even if that extra byte is typically a string nil terminator.

[kzc]

Despite my initial objection, @petkaantonov would you mind putting the base64_decode valgrind fix at the top of this ticket into #2133 since it can only be reproduced in JS easily on that branch? I think you'll agree it's an obvious fix.

[petkaantonov]

sure

[kzc]

Fix on worker threads branch #2133.