Fixed off-by-two error in json module #835

Closed
wants to merge 2 commits into
from

Projects

None yet

2 participants

@qnet-herwin
Contributor

When using rlm_rest with JSON requests, the bug was triggered if the value including the trailing JSON data was at a chunk boundary (eg, when the closing } of the value was at position 1023). The bug was pretty rare to trigger, and resulted in omission of the value in the JSON data.

Most likely the cause of this bug was code reuse from other parts, but when calculating the free space the trailing JSON data wasn't accounted for. I haven't been able to reproduce the bug with this patch.

I haven't tested this with the master branch, but I assume the problem exists there as well.

qnet-herwin added some commits Nov 17, 2014
@qnet-herwin qnet-herwin Fixed off-by-two error in json module
When using rlm_rest with JSON requests, the bug was triggered if the value including the trailing JSON data was at a chunk boundary (eg, when the closing `}` of the value was at position 1023). The bug was pretty rare to trigger, and resulted in omission of the value in the JSON data.

Most likely the cause of this bug was code reuse from other parts, but when calculating the free space the trailing JSON data wasn't accounted for. I haven't been able to reproduce the bug with this patch.
3d4ce69
@qnet-herwin qnet-herwin Fix typo in comment in rest.c 21c2c73
@arr2036
Member
arr2036 commented Nov 21, 2014

No, I don't think you have the bug quite diagnosed correctly, unless you're seeing value corruption?

I can see that if the value was on a chunk boundary, and it was a multivalued attribute, and there wasn't enough space to write the trailing comma, then that would cause the value to be skipped.

@arr2036 arr2036 closed this in cdc1a62 Nov 21, 2014
@qnet-herwin
Contributor

Nope, this doesn't fix the problem.

I've got it reproduced by adding some dummy data to request, then adding an extra attribute after that. The closing } of that last attribute should be the 1024th byte. The logging looks like this:

Fri Nov 21 09:26:06 2014 : Debug: (0) rest: Encoding attribute "Tmp-String-0"
Fri Nov 21 09:26:06 2014 : Debug: (0) rest:   Type   : string
Fri Nov 21 09:26:06 2014 : Debug: (0) rest:   Length : 225
Fri Nov 21 09:26:06 2014 : Debug: (0) rest:   Value  : "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
Fri Nov 21 09:26:06 2014 : Debug: (0) rest:   Length : 225
Fri Nov 21 09:26:06 2014 : Debug: (0) rest:   Value  : "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"
Fri Nov 21 09:26:06 2014 : Debug: (0) rest:   Length : 116
Fri Nov 21 09:26:06 2014 : Debug: (0) rest:   Value  : "cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc"
Fri Nov 21 09:26:06 2014 : Debug: (0) rest: Encoding attribute "Tmp-String-1"
Fri Nov 21 09:26:06 2014 : Debug: (0) rest:   Type   : string
Fri Nov 21 09:26:06 2014 : Debug: (0) rest:   Length : 3
Fri Nov 21 09:26:06 2014 : Debug: (0) rest:   Value  : "bob"
Fri Nov 21 09:26:06 2014 : Debug: (0) rest: JSON Data: {"User-Name":{"type":"string","value":["bob"]},"User-Password":{"type":"string","value":["hello"]},"NAS-IP-Address":{"type":"ipaddr","value":["127.0.1.1"]},"NAS-Port":{"type":"integer","value":[0]},"Event-Timestamp":{"type":"date","value":["Nov 21 2014 09:26:06 CET"]},"Message-Authenticator":{"type":"octets","value":["0xdfdca94733e49ae371a38c979335a3fb"]},"Tmp-String-0":{"type":"string","value":["aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb","cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc"]},"Tmp-String-1":{"type":"string","value":[
Fri Nov 21 09:26:06 2014 : Debug: (0) rest: Returning 1017 bytes of JSON data (buffer full or chunk exceeded)
Fri Nov 21 09:26:06 2014 : Debug: (0) rest: JSON Data: }
Fri Nov 21 09:26:06 2014 : Debug: (0) rest: Returning 1 bytes of JSON data

Before the chunk exceeded message, it reaches https://github.com/FreeRADIUS/freeradius-server/blob/v3.0.x/src/modules/rlm_rest/rest.c#L793 where freespace equals 1. Here, it skips to no-space, and doesn't add the attribute to the string anymore. Dumping the structure that's being sent on the wire shows that it ends with "Tmp-String-1":{"type":"string","value":[}, just as you would expect with this debug log.

@arr2036 arr2036 reopened this Nov 21, 2014
@arr2036
Member
arr2036 commented Nov 21, 2014

Alright, i'll try and reproduce it locally.

@arr2036
Member
arr2036 commented Nov 21, 2014

Ok, reproduced it locally.

@arr2036 arr2036 closed this in ffe6f5d Nov 21, 2014
@arr2036
Member
arr2036 commented Nov 21, 2014

It was two separate issues, should be fixed now

@qnet-herwin
Contributor

Nope, still got the exact same output as before.

@arr2036
Member
arr2036 commented Nov 24, 2014

Could you call the debug_request policy https://github.com/FreeRADIUS/freeradius-server/blob/master/raddb/policy.d/debug
and paste the output, so I can use the exact same attributes.

@arr2036 arr2036 reopened this Nov 25, 2014
@qnet-herwin
Contributor

https://gist.github.com/qnet-herwin/00c9d22a1ccec8d16ab0

I don't think this was the output you expected ;)

@qnet-herwin
Contributor
User-Name = 'bob'
User-Password = 'hello'
NAS-IP-Address = 127.0.1.1
NAS-Port = 0
Called-Station-Id = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
Called-Station-Id += aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
Called-Station-Id += aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
Called-Station-Id += aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
NAS-Identifier = foo
Message-Authenticator = 0x00
User-Name = 'bob'
User-Password = 'hello'
NAS-IP-Address = 127.0.1.1
NAS-Port = 0
Called-Station-Id = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
Called-Station-Id += aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
Called-Station-Id += aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
Called-Station-Id += aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
NAS-Identifier = foo
Message-Authenticator = 0x00

These are two requests that trigger the bug at an almost vanilla install. The can be piped to radclient. The attribute Called-Station-Id is just used for padding; the atribute doesn't have any particular meaning here. The second one has one byte extra in Called-Station-Id.
The attribute NAS-Identifier is the attribute that gets truncated in the JSON-request, once again a random attribute with no semantic meaning here. I have a diff of the config on https://gist.github.com/qnet-herwin/0371a2c72a703529cc00 Tested with version 3.0.x (commit a2ab465)

@arr2036
Member
arr2036 commented Nov 25, 2014
aaaaaaaaa"
Tue Nov 25 09:40:40 2014 : Debug: (1) rest:   Length : 170
Tue Nov 25 09:40:40 2014 : Debug: (1) rest:   Value  : "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
Tue Nov 25 09:40:40 2014 : Debug: (1) rest: Encoding attribute "NAS-Identifier"
Tue Nov 25 09:40:40 2014 : Debug: (1) rest:   Type   : string
Tue Nov 25 09:40:40 2014 : Debug: (1) rest:   Length : 3
Tue Nov 25 09:40:40 2014 : Debug: (1) rest:   Value  : "foo"
Tue Nov 25 09:40:40 2014 : Debug: (1) rest: JSON Data: {"User-Name":{"type":"string","value":["bob"]},"User-Password":{"type":"string","value":["hello"]},"NAS-IP-Address":{"type":"ipaddr","value":["127.0.1.1"]},"NAS-Port":{"type":"integer","value":[0]},"Called-Station-Id":{"type":"string","value":["aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"]},"NAS-Identifier":{"type":"string","value":["foo"
Tue Nov 25 09:40:40 2014 : Debug: (1) rest: Returning 1022 bytes of JSON data (buffer full or chunk exceeded)
Tue Nov 25 09:40:40 2014 : Debug: (1) rest: Encoding attribute "Message-Authenticator"
Tue Nov 25 09:40:40 2014 : Debug: (1) rest:   Type   : octets
Tue Nov 25 09:40:40 2014 : Debug: (1) rest:   Length : 34
Tue Nov 25 09:40:40 2014 : Debug: (1) rest:   Value  : "0x0bd825aa032776a13e72dfd33c25e5ed"
Tue Nov 25 09:40:40 2014 : Debug: (1) rest: JSON Data: ]},"Message-Authenticator":{"type":"octets","value":["0x0bd825aa032776a13e72dfd33c25e5ed"]}}
Tue Nov 25 09:40:40 2014 : Debug: (1) rest: Returning 92 bytes of JSON data
Tue Nov 25 09:40:40 2014 : Debug: (1) rest: Processing response header
Tue Nov 25 09:40:40 2014 : Debug: (1) rest:   Status : 100 (Continue)
Tue Nov 25 09:40:40 2014 : Debug: (1) rest: Continuing...

There we go. Just wasn't updating the encoded pointer.

@arr2036 arr2036 closed this Nov 25, 2014
@qnet-herwin qnet-herwin deleted the unknown repository branch Nov 25, 2014
@qnet-herwin
Contributor

Yes, that works, and the solution is cleaner than what I proposed. Do you still want a separate issue for the double-free error when calling debug_request?

@arr2036
Member
arr2036 commented Nov 25, 2014

Yes please. If you can narrow it down to a particular attribute/value that'd be helpful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment