Stack-Buffer-Overflow in snmp.c:1639

Drawishe commented 4 months ago

Describe the bug

I have found Stack-Buffer-overflow error with cups upstream version (462e851), using unittest testsnmp.c as a harness for fuzzing. Here are the files needed to reproduce the bug:

repro.tar.gz

To Reproduce

Steps to reproduce the behaviour:

move given argv-fuzz-inl.h and crash.input files in cups/ directory
build project with clang and ASAN

export CC=clang-17
export CXX=clang++-17
export CFLAGS="-fPIC -fprofile-instr-generate -fcoverage-mapping -fsanitize=address -g"
export CXXFLAGS="-fPIC -fprofile-instr-generate -fcoverage-mapping -fsanitize=address -g"
export LDFLAGS="-g -fsanitize=address"
./configure --enable-static --disable-shared
make

Modify testsnmp.c file

Add given library

#include "argv-fuzz-inl.h"

Add AFL_INIT_ARGV() macro in the beggining of main function

int                 /* O - Exit status */
main(int  argc,             /* I - Number of command-line args */
     char *argv[])          /* I - Command-line arguments */
{
  AFL_INIT_ARGV();

build testsnmp target

cd cups
make testsnmp

run testsnmp with given input

./testsnmp < crash.input 
_cupsSNMPDefaultCommunity: PASS (public)
_cupsSNMPOpen: PASS
=================================================================
==3481330==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fb9de202948 at pc 0x5622f1cc05b5 bp 0x7ffffa4e7600 sp 0x7ffffa4e75f8
READ of size 4 at 0x7fb9de202948 thread T0
    #0 0x5622f1cc05b4 in asn1_size_oid /home/as/cups-opensource-2.4.7/cups/snmp.c:1639:8
    #1 0x5622f1cbee42 in asn1_encode_snmp /home/as/cups-opensource-2.4.7/cups/snmp.c:1078:13
    #2 0x5622f1cbeb94 in _cupsSNMPWrite /home/as/cups-opensource-2.4.7/cups/snmp.c:670:11
    #3 0x5622f1cab757 in show_oid /home/as/cups-opensource-2.4.7/cups/testsnmp.c:230:10
    #4 0x5622f1cab168 in main /home/as/cups-opensource-2.4.7/cups/testsnmp.c:95:15
    #5 0x7fb9dfc461c9 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #6 0x7fb9dfc46284 in __libc_start_main csu/../csu/libc-start.c:360:3
    #7 0x5622f1bd2070 in _start (/home/as/cups-opensource-2.4.7/cups/testsnmp+0x62070) (BuildId: 44a27014a37225ec978189dfbbef94f66dfa2be8)

Address 0x7fb9de202948 is located in stack of thread T0 at offset 2376 in frame
    #0 0x5622f1cbe6dd in _cupsSNMPWrite /home/as/cups-opensource-2.4.7/cups/snmp.c:621

  This frame has 3 object(s):
    [32, 2376) 'packet' (line 623) <== Memory access at offset 2376 overflows this variable
    [2512, 3984) 'buffer' (line 624)
    [4112, 4368) 'temp' (line 627)
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow /home/as/cups-opensource-2.4.7/cups/snmp.c:1639:8 in asn1_size_oid
Shadow bytes around the buggy address:
  0x7fb9de202680: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7fb9de202700: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7fb9de202780: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7fb9de202800: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7fb9de202880: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x7fb9de202900: 00 00 00 00 00 00 00 00 00[f2]f2 f2 f2 f2 f2 f2
  0x7fb9de202980: f2 f2 f2 f2 f2 f2 f2 f2 f2 f2 00 00 00 00 00 00
  0x7fb9de202a00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7fb9de202a80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7fb9de202b00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7fb9de202b80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==3481330==ABORTING

This error is caused by the lack of checking when executing the for cycle. The for cycle ends when the value of *oid < 0. In this case, there are no conditions for cycle termination, which causes a stack-buffer-overflow.

https://github.com/OpenPrinting/cups/blob/462e85146773486be3bf82af5e99a89caeb188c3/cups/snmp.c#L1638-L1640

Solution:

A possible solution to the problem is to add a counter to the for loop that terminates the loop when the limit number of oid elements is reached (In testsnmp.c this value == 128).

System information:

OS and its version: debian, 12
CUPS version: upstream (462e851)

zdohnal commented 4 months ago

Hi @Drawishe ,

thank you for the report!

I've looked into the code and I'm not sure how stack overflow happened there - we add limit (-1) in _cupsSNMPWrite() and _cupsSNMPStringToOID() into oid:

_cupsSNMPStringToOID():

 502  /*
 503   * Terminate the end of the OID array and return...
 504   */
 505 
 506   dstptr[1] = -1;
 507 
 508   return (dst);

_cupsSNMPWrite():

 658   for (i = 0; oid[i] >= 0 && i < (CUPS_SNMP_MAX_OID - 1); i ++)
 659     packet.object_name[i] = oid[i];
 660   packet.object_name[i] = -1;

The check in _cupsSNMPWrite() protects from larger oids than CUPS_SNMP_MAX_OID as well.

Then packet is sent to asn1_encode_snmp():

 669 
 670   bytes = asn1_encode_snmp(buffer, sizeof(buffer), &packet);
 671

and its packet->object_name is used oid - it looks to me that the array cannot be bigger than 128 and it has -1 as last member.

Can you explain when the stack overflow happens?

Drawishe commented 4 months ago

Hello, @zdohnal! Thank you for your comment. I spent some time to debug this issue, and get some information. In this case, the number of array elements oid == 0. The asn1_size_oid() function checks the element oid[1]. https://github.com/OpenPrinting/cups/blob/462e85146773486be3bf82af5e99a89caeb188c3/cups/snmp.c#L1635 In this case, oid[1] == 0, because the packet structure is completely filled with 0 before creating SNMP message. https://github.com/OpenPrinting/cups/blob/462e85146773486be3bf82af5e99a89caeb188c3/cups/snmp.c#L649

I think, we also need to check the oid[0] for it's value.

zdohnal commented 4 months ago

Ok, I looked into more...

The reason why the array does have limit on the first number is because oid from file is invalid (there are no dots and the number is too large, so integer overflows).

Since the affected function is not public and accessible only via _cupsSNMPWrite() (another private function), so we can only ensure the input is valid for the function. OID is static arrays in most cases, the only cases when we load it from input it is in the test and when reading from side channel - both can be fixed by _cupsSNMPStringtoOID().

I'll file PR about it.

OpenPrinting / cups