Skip to content

Add bounds check to GetIntPositive#6676

Closed
guidovranken wants to merge 1 commit intowolfSSL:masterfrom
guidovranken:getintpositive-bounds-check
Closed

Add bounds check to GetIntPositive#6676
guidovranken wants to merge 1 commit intowolfSSL:masterfrom
guidovranken:getintpositive-bounds-check

Conversation

@guidovranken
Copy link
Copy Markdown
Contributor

@guidovranken guidovranken commented Aug 7, 2023

Description

Add bounds check to asn.c GetIntPositive() to ensure subsequent buffer access is within array bounds.

ZD 16549

Testing

Fuzzing

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

@wolfSSL-Bot
Copy link
Copy Markdown

Can one of the admins verify this patch?

@JacobBarthelmeh
Copy link
Copy Markdown
Contributor

JacobBarthelmeh commented Aug 7, 2023

ok to test, retest this please Jenkins

Comment thread wolfcrypt/src/asn.c
if (ret != 0)
return ret;

if (idx < 1 || idx >= maxIdx) {
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.

Can't check for 1 as the starting index may not be zero.
GetASNInt ensures that a header (2 bytes) is read.
The length may be 0 though. and this is a problem.

The index won't be greater than maxIdx but the idx + length is not checked.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Can you suggest a change please?

Copy link
Copy Markdown
Contributor

@SparkiDev SparkiDev Aug 9, 2023

Choose a reason for hiding this comment

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

Checked the code again and GetASNInt calls GetASNHeader and that will check that the length corresponds to bytes in the buffer. So no need to check: idx + length > maxIdx.

if (length > 0) {
   /* Check that the preceding byte is zero when top bit set. */
   if (((input[idx] & 0x80) == 0x80) && (input[idx - 1] != 0x00))
         return MP_INIT_E;
}

Note that when the first byte of data has the top bit set, the preceding byte is the length and must be > 0.

@SparkiDev
Copy link
Copy Markdown
Contributor

Different fix in #6739

@SparkiDev SparkiDev closed this Aug 30, 2023
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.

4 participants