base64: fix out-of-bounds write in htp_base64_decode#461
base64: fix out-of-bounds write in htp_base64_decode#461zhangjy1014 wants to merge 1 commit intoOISF:0.5.xfrom
Conversation
|
Hi @catenacyber, please take a look |
|
Thanks for the work. |
|
Hi @catenacyber, thanks for your review, I've added several tests. The patched version passes all of them, while the unpatched version triggers an ASan error on each of them. |
|
Could you squash together all the commits ? |
…tput buffer (*plainchar) to store the partial result of the next byte before verifying if the output buffer had sufficient space. This resulted in a heap-buffer-overflow when the provided output buffer was undersized or exactly full. This patch fixes the issue by: Moving the partial byte assignment after the buffer length check. Correctly updating the decoder state (decoder->step and decoder->plainchar) when the buffer limit is reached, ensuring the state is preserved without writing out-of-bounds. Fixes OISF#458 This patch is generated by ASKRepair, an agentic automated vulnerability repair framework
zhangjy1014
force-pushed
the
fix-issue-458
branch
from
ffd2439 to
6cb45cb
Compare
squashed, thanks! |
|
Since this fixes an out-of-bounds write issue, could you help merge it when convenient? |
|
The commit message should be updated : the first line should be much shorter: like As for merging, I do not know, you can see I have #460 pending as well ;-) |
catenacyber
left a comment
catenacyber
left a comment
There was a problem hiding this comment.
This is good for me @victorjulien
jufajardini
left a comment
jufajardini
left a comment
There was a problem hiding this comment.
Please see my inline comments, and also please consider Victor (#463 (comment)) and Philippe's feedback on the commit format, it's not following our standard. (you can see c990dc3 for an example)
| bstr_free(out); | ||
| } | ||
|
|
||
| // Tests for issue #458: htp_base64_decode out-of-bounds write with undersized output buffer |
There was a problem hiding this comment.
If we have this comment, we should also have one indicating where do such tests end. :P
| int ret = htp_base64_decode(&dec, in, 4, out, 1); | ||
| EXPECT_EQ(1, ret); |
There was a problem hiding this comment.
Here and in similar calls, can't we simply call htp_base64_decode from EXPECT_EQ as it's done for other tests?
Thanks for your work, but we need some further improvements to make merging possible :) |
4 participants
The htp_base64_decode function previously performed a write to the output buffer (*plainchar) to store the partial result of the next byte before verifying if the output buffer had sufficient space. This resulted in a heap-buffer-overflow when the provided output buffer was undersized or exactly full.
This patch fixes the issue by:
Fixes #458
This patch is generated by ASKRepair, an agentic automated vulnerability repair framework