cisco / cjose

C library implementing the Javascript Object Signing and Encryption (JOSE)
MIT License
103 stars 60 forks source link

In _decode function, memset the allocated memory to 0 before initializing it #74

Open mysticlife1111 opened 6 years ago

mysticlife1111 commented 6 years ago

In File cjose/src/base64.c

In function static inline bool _decode(const char *input, size_t inlen, uint8_t *output, size_t outlen, bool url, cjose_err *err)

Line 78: uint8_t buffer = cjose_get_alloc()(sizeof(uint8_t) rlen);

Fix: memset(buffer, 0, sizeof(uint8_t) * rlen);

Symptoms: I am using function

jws = cjose_jws_import(access_token, strlen(access_token), err); to get the jws object and then to get the plaintext version i am using cjose_jws_get_plaintext(jws, &plaintext, &plaintext_len, err)

However, when i see the plaintext version, it has extra garbage characters in the end after the termination of the JSON

i am using the JSON parse to get the values, C function is e = cl_json_parse(a_token, &json_top);

It is throwing error because of the corrupted plaintext field (extra garbage character), as i can't change the source code of cjose, workaround is parse from the end till i get the closing } brace to get the final length.

One weird thing is "first time when i decode the string then the JSON plaintext looks ok, its subsequent time when the garbage characters shows up" not sure if it is due to the alloc function.

Please memset the buffer to 0 before using it.

balthorium commented 6 years ago

@mysticlife1111 the outputs of both cjose_jws_get_plaintext() and _decode() functions are byte buffers of explicit length, not zero-terminated character strings. You may want to check your code for a buffer over-read error, which could happen if you are treating the plaintext as a zero-terminated string when the producer of the JWS did not include a terminating zero in the original payload.

If you're certain this is not the case, though, it would be great if you could share a small runnable example of code (with imported JWS) that demonstrates the problem.

mysticlife1111 commented 6 years ago

Hi,

cjose_jws_t jws = NULL; size_t plaintext_len = 0; uint8_t plaintext = NULL;

I am using this function

jws = cjose_jws_import(access_token, strlen(access_token), err);

/ Get the Plaintext information / if (cjose_jws_get_plaintext(jws, &plaintext, &plaintext_len, err)) {

So i am expecting that plaintext_len will give me the right length of the plaintext,

Here is the decoded JWT in plaintext {"jti":"e9fd6b08-5804-4e29-8787-1d241dc5cb40","iss":"eCDM AAA Service","iat":1523404709,"exp":1600000020,"ext":{"system":{"role":["System Admin"]},"audit":{"uid":"00000000-0000-4000-a000-000000000000"}},"sub":"terasa@abc.com","authorization-token-bitmap":{"username":"admin","authenticated":true,"id":"00000000-0000-4000-a000-000000000000","userType":"LOCAL","timestamp":0,"creationTime":0,"tenantScope":"/00000000-0000-4000-b000-000000000000/00000000-0000-4000-a000-000000000000","authorities":[{"tenants":["00000000-0000-4000-a000-000000000000","00000000-0000-4000-b000-000000000000"],"privileges":["68719476735"],"roles":["27815934-7826-4e9c-b53d-99d5ac045ffc"]}]},"rti":"f20a4492-530f-4aaa-9a77-0428c9a954b9"}È:F:^U^?

È:F:^U^? are the extra characters, this causes error when i use the JSON parser as it complains JSON string is wrong.

I have to get the actual plaintext len as follows / Remove the trailing garbage from the plaintext before JSON parsing / while (plaintext_len != 0) { if (plaintext[plaintext_len-1] == '}') { break; } plaintext_len = plaintext_len - 1; }

I am releasing the memory

if (jws) { cjose_jws_release(jws); jws = NULL; plaintext = NULL; plaintext_len = 0; }

Seems like cjose_jws_release(jws); will release the plaintext memory as well.

zandbelt commented 6 years ago

Use strlen(access_token)+1 to include the trailing \0 in the decoded result.

mysticlife1111 commented 6 years ago

I tried using strlen(access_token) + 1 but now i am getting error in the function cjose_jws_import

linuxwolf commented 6 years ago

If jws_get_plaintext() is not setting plaintext_len to the right length, that is a bug we need to fix. Zeroing out all the memory, while a good idea, shouldn't be solving the problem. Despite the name, plaintext is binary data, and NULL-terminating doesn't make sense in that case.

The referenced cl_json_parse doesn't take a length, so I assume you are modifying plaintext to add a NULL-terminated value? I'm not sure we've gotten enough context for how you're working with things.

Also, yes cjose_jws_release() does release the plaintext memory, as the ownership belongs to the cjose_jws_t. You'll need to copy it to another buffer you own the memory of if you need to keep it around.

mysticlife1111 commented 6 years ago

This is the list of functions i am using

  1. jws = cjose_jws_import(access_token, strlen(access_token), err);
  2. cjose_jws_get_plaintext(jws, &plaintext, &plaintext_len, err)
  3. e = cl_json_parse(a_token, &json_top);

In the second step, plaintext_len gives length including garbage in the end and the step 3 fails. So had to get the right length in plaintext_len removing the garbage characters in the end.

strlen(access_token) + 1 doesn't work!

linuxwolf commented 6 years ago

@mysticlife1111 thank you for the context. I'll admit I'm puzzled why plaintext_len is invalid for you but the tests pass. If you have a chance, please try make test and let us know if that succeeds or fails. In the meantime, I'm taking another look at our existing tests to see where gaps might exist.

I'm also curious how your a_token is initialized. I'm assuming there is code between your steps one and two that takes the bytes pointed to by plaintext are then pointed to by a_token, with NULL termination.

mysticlife1111 commented 6 years ago

Hello,

yes

First getting the jws Step 1: jws = cjose_jws_import(access_token, strlen(access_token), err);

after getting the plaintext

Step 2: cjose_jws_get_plaintext(jws, &plaintext, &plaintext_len, err);

Step 3: / Remove the trailing garbage from the plaintext before JSON parsing / while (plaintext_len != 0) { if (plaintext[plaintext_len-1] == '}') { break; } plaintext_len = plaintext_len - 1; }

Step 4: a_token = calloc(1, plaintext_len + 1); panic_if_null(a_token);

Step 5: snprintf(a_token, plaintext_len + 1, "%s", plaintext);

Step 6: / Now parse the plaintext JSON to get JSON top object / e = cl_json_parse(a_token, &json_top); if (e != DD_OK) { // error }

Regards!