-
Notifications
You must be signed in to change notification settings - Fork 181
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
snmp: Validate input data #431
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comments below. I assume this will also reproduce on 64-bit platforms (haven't tested yet) so we definitely want to fix the bounds checking.
@@ -1230,14 +1230,16 @@ asn1_get_integer( | |||
unsigned char *bufend, /* I - End of buffer */ | |||
unsigned length) /* I - Length of value */ | |||
{ | |||
int value; /* Integer value */ | |||
unsigned value; /* Integer value */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this change is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is required to use an unsigned type here to avoid undefined behavior. You can compile cups with CFLAGS="-fsanitize=undefined" LDFLAGS="-fsanitize=undefined" ./configure
and use the following byte sequence in my proof of concept, also on 64 bit systems: b'\x30\x07\x02\x04\xC0\x00\x00\x00'
The problem is in line 1249: value = ((value & 0xffffff) << 8) | **buffer;
If value is 0xFFFFFFFF then it is mapped to 0x00FFFFFF and shifted by 8 bits to the left, i.e. 0xFFFFFF00. This shift is undefined behavior because the signed integer turned from very large to negative, i.e. a signed integer overflow occurred.
By using an unsigned data type the problem is circumvented.
cups/snmp.c
Outdated
|
||
|
||
if (*buffer >= bufend) | ||
return (0); | ||
|
||
if (length > sizeof(int)) | ||
{ | ||
if (length > bufend - *buffer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Best to put bufend - *buffer
in parenthesis here. But really since all other accesses are bounds-checked it isn't necessary to peg the buffer pointer to the end of the buffer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bounds check itself is the problem on 32 bit systems: The length can be as large as UINT_MAX. If you add UINT_MAX to a pointer on a 32 bit system, it is effectively subtracting the pointer by 1. As long as the calculation overflows, *buffer will be smaller than bufend, most likely even smaller than the initial *buffer value. This allows to move *buffer to any position in virtual memory. And future bounds-checks in the code path do not stop this illegal memory access.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I have added the parentheses at this and the other places which I have modified.
cups/snmp.c
Outdated
@@ -1285,6 +1287,8 @@ asn1_get_length(unsigned char **buffer, /* IO - Pointer in buffer */ | |||
length = (length << 8) | **buffer; | |||
} | |||
|
|||
if (length > bufend - *buffer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since all of the other functions check that *buffer
is less than bufend
, I'm not sure this is necessary, either...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see above for explanation. It could be that one of these checks can be omitted because the other is in place. Since I am not confident enough to see every possible combination, I preferred to have too many checks in place.
@@ -1355,7 +1359,7 @@ asn1_get_packed( | |||
unsigned char **buffer, /* IO - Pointer in buffer */ | |||
unsigned char *bufend) /* I - End of buffer */ | |||
{ | |||
int value; /* Value */ | |||
unsigned value; /* Value */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, no need for this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my previous explanation. Example byte sequence for my proof of concept is b'\x1F\xFF\xFF\xFF\xFF\xFF'
.
The snmp code does not prevent overflows during pointer arithmetics on 32 bit architectures. Check length before performing additions. This can lead to out of boundary accesses or endless loops if debug output is enabled (proof of concept exists for OOB access). Proof of Concept: 1. Create and run server with crashing payload ``` cat > poc.py << EOF from socket import AF_INET, SOCK_DGRAM, socket s=socket(AF_INET, SOCK_DGRAM) s.bind(('',161)) print('Waiting for incoming SNMP data') m, addr=s.recvfrom(1024) print('Replying with crash payload') s.sendto(b'\x30\x07\x02\x84\xDE\xAD\xBA\xBE\x00', addr) EOF sudo python poc.py ``` 2. Run snmp backend (or try to "Add Printer") `/usr/lib/cups/backend/snmp localhost`
Shifting bits into highest bit of integers is undefined behavior according to C standards. It can be easily prevented by using an unsigned type during shifts, casting to signed at the end.
@michaelrsweet It does not fix the actual overflow issue on 32 bit systems though. Please see my explanation. I hope I was able to clarify a bit more what happens. To better understand my proof of concept code, let me add this comment. I hope it helps:
|
@ferivoz I'm not so concerned about the "undefined behavior" since we are masking the values - casting from unsigned to signed won't actually "fix" undefined integer behavior anyways, and the wording in the C standard is only there to support odd-word-size processors that we already don't support... I'll update with length checks, as I hadn't considered wraparound for pointer values. |
@ferivoz Have my changes satisfied your security concerns? |
@michaelrsweet, your changes look good to me. Thank you for taking care of this! |
It is possible to trigger a crash or endless loop in snmp on
32 bit architectures due to missing integer validation. It should
not be possible on 64 bit architectures if I am not mistaken.
I have also adjusted the code to prevent signed integer overflows
which are harmless in reality but undefined in C standards.
It would take compiler options on real hardware to let the program
crash.
Proof of Concept:
/usr/lib/cups/backend/snmp localhost
x