Skip to content

detect/cip: add check for sscanf return-value - v5 #10433

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
0xEniola wants to merge 1 commit into from
Closed

detect/cip: add check for sscanf return-value - v5 #10433

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/detect-cipservice.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,10 @@ static DetectCipServiceData *DetectCipServiceParse(const char *rulestrc)
goto error;
}

sscanf(token, "%2" SCNu8, &var);
if (sscanf(token, "%2" SCNu8, &var) != 1) {
Copy link
Member

@victorjulien victorjulien Feb 26, 2024

Choose a reason for hiding this comment

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

it looks like the original code has some serious issues, c5cf296 broke things by turning the input array into a uint8_t array, even though for class and attribute we're supposed to have a uint16_t values. So that would need fixing.

In general, on further inspection, it's unclear why we have the sscanf at all. We already have num, and it's value is validated to be in range, so we can safely cast it to uint8_t for service and uint16_t for the other two.

cc @catenacyber

Copy link
Contributor

@jufajardini jufajardini Feb 26, 2024

Choose a reason for hiding this comment

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

Complementing this: a good thing to have to go along the next PR for this would be a Suricata-verify PR that used the cip.class and cip.attribute keywords with values that would not fit a u8 so we check that this is working as expected.

Copy link
Contributor

@catenacyber catenacyber Feb 26, 2024

Choose a reason for hiding this comment

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

Indeed thanks for the analysis Victor.

We can remove the call to sscanf and use num instead of var

Copy link
Contributor Author

@0xEniola 0xEniola Feb 26, 2024

Choose a reason for hiding this comment

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

So to remove sscanf and do input[i++] = num; then.

And do an SV test.

Copy link
Contributor

@catenacyber catenacyber Feb 26, 2024

Choose a reason for hiding this comment

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

Yes, and pay attention to use the right integer types uint16 and uint8 ;-)

Copy link
Contributor

@catenacyber catenacyber Feb 26, 2024

Choose a reason for hiding this comment

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

I meant on the Suricata patch

Copy link
Contributor Author

@0xEniola 0xEniola Feb 26, 2024

Choose a reason for hiding this comment

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

Oh! I thought I'm only to modify the sscanf part and the input[i++] = var part.

Copy link
Contributor

@catenacyber catenacyber Feb 26, 2024

Choose a reason for hiding this comment

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

I think some compilers will warn on input[i++] = num;

Copy link
Contributor Author

@0xEniola 0xEniola Feb 26, 2024

Choose a reason for hiding this comment

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

So to change the definition if the input array to uint16_t then.

Copy link
Contributor

@catenacyber catenacyber Feb 26, 2024

Choose a reason for hiding this comment

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

indeed

SCLogError("incorrect input format; token %s should be uint8", token);
goto error;
}
input[i++] = var;

token = strtok_r(NULL, delims, &save);
Expand Down