Deallocation of control->suffix corrupts Heap Memory

[pietroborrello]

The suffix field in the static rzip_control structure is initialized to point to global memory in initialize_control

https://github.com/ckolivas/lrzip/blob/64eb4a8c37aead0b473a06d515bc348f23f3a1e7/lrzip.c#L1341

and in the lrzip main.

https://github.com/ckolivas/lrzip/blob/6a1600b3541bec0bd087d6f58a8619cd4a76c33f/main.c#L496

However the field is then treated as a heap allocated variable while freeing the rzip_control variable. Both in rzip_control_free

https://github.com/ckolivas/lrzip/blob/465afe830f1432fbccb0e228da1f6e7ed4e79653/rzip.c#L1269

and when setting a new suffix

https://github.com/ckolivas/lrzip/blob/465afe830f1432fbccb0e228da1f6e7ed4e79653/liblrzip.c#L439

Impact

Corrupting the heap state may result in an exploitable vulnerability, especially if initialized with optarg that points to global RW memory.

Fix It is sufficient to initialize control->suffix using the return value of a strdup of the strings.

[pete4abw]

Good grief! This has been around since v0.1 and rzip before, even before I became involved (v0.19). The initialise function should be used for setting constants or like-size variables, like compression level, etc. Setting control->suffix to equal optarg is probably a mistake if there will be recursion. I think the dealloc of suffix is incorrect too. It does not need to be. HOWEVER, the ability to pipe input to lrzip sort of makes recursion obsolete and unnecessary. strdup will work and I'll see about implementing it in lrzip-next. Thank you

[pietroborrello]

Great, thank you! Will checkout lrzip-next

[ckolivas]

Fixed in master.

[carnil]

Retrospective note: This seems to have been a CVE assigned, which is CVE-2022-28044.

[utkarsh2102]

Hello, is there a simple reproducer for this one?