Skip to content

util: fix deserialize buffer overflow#74

Closed
alonbl wants to merge 1 commit into
OpenSC:masterfrom
alonbl:deserialize
Closed

util: fix deserialize buffer overflow#74
alonbl wants to merge 1 commit into
OpenSC:masterfrom
alonbl:deserialize

Conversation

@alonbl
Copy link
Copy Markdown
Member

@alonbl alonbl commented Nov 6, 2025

Missing check for source file increment.

Reported-by: Aarnav Bos aarnav@srlabs.de

Copy link
Copy Markdown
Contributor

@selvanair selvanair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a simpler version which I hope is correct :)
https://gist.github.com/selvanair/e541faacac36a40f1066b091e87084de

Comment thread lib/pkcs11h-util.c Outdated
else {
if (t != NULL) {
if (n+1 > *max) {
s++;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If t == NULL, this can advance past the end of s.

Comment thread lib/pkcs11h-util.c Outdated
s++;

if (t != NULL && n < m) {
if (b2 = _ctoi(*s) == -1) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be ((b2 = _ctoi(*s)) == -1)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah!!! thank you for finding it.

Comment thread lib/pkcs11h-util.c Outdated
t++;
}
n++;
s++;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can advance past the end if t is NULL or n >= m.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for reviewing... bummer I wanted to avoid logic when t == NULL...

Missing check for source file increment.

Reported-by: Aarnav Bos <aarnav@srlabs.de>
@alonbl
Copy link
Copy Markdown
Member Author

alonbl commented Nov 7, 2025

@selvanair thank you so much for the review, I've modified the implementation to use macros, I hope now all is ok.

@selvanair
Copy link
Copy Markdown
Contributor

selvanair commented Nov 7, 2025

Looks good to me if returning a wrong value of *max is okay in case of error. Let's see whether the fuzzer can still crash it!

@R9295
Copy link
Copy Markdown

R9295 commented Nov 10, 2025

@selvanair @alonbl the fix also looks good from the fuzzing side!

@alonbl
Copy link
Copy Markdown
Member Author

alonbl commented Nov 10, 2025

Thank you all!

@alonbl alonbl closed this Nov 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants