detect/cip: add check for sscanf return-value - v5 by 0xEniola · Pull Request #10433 · OISF/suricata

0xEniola · 2024-02-15T10:47:51Z

Link to redmine ticket: https://redmine.openinfosecfoundation.org/issues/6753

Previous PR: #10418

Describe changes:

Format specifier used on message for SCLogError re: sscanf [line 161] was wrong.

Found by CodeQL. Cf https://codeql.github.com/codeql-query-help/cpp/cpp-missing-check-scanf/ The sscanf function returns the number of input items successfully matched and assigned. We have to ensure that all subsequent uses of scanf output arguments occur in a branch of an if statement (or similar), in which it is known that the corresponding scanf call has in fact read all possible items from its input. Hence, we always have to check if sscanf runs successfully by comparing the return value to a numerical constant. Ticket: 6753

victorjulien · 2024-02-15T11:22:13Z

Can you show some examples of Suricata's output with malformed rules that display this error?

0xEniola · 2024-02-15T14:36:21Z

Can you show some examples of Suricata's output with malformed rules that display this error?

When I tested using unit tests, I found out with all the inputs I tried, the checks in the while case at line 113, caught all wrong user inputs before getting to the sscanf part, so there was no point where it got triggered.

0xEniola · 2024-02-18T08:47:07Z

Can you show some examples of Suricata's output with malformed rules that display this error?

Had to remove the other if and else if statements in the while statement, and left only the one with sscanf.

Test DetectCipServiceParseTest01                                  : Debug: time: offline time mode enabled [TimeModeSetOffline:util-time.c:108]
Debug: time: time set to 1708245819 sec, 436821 usec [TimeSet:util-time.c:133]
Error: detect-cipservice: incorrect input format; token xxmp should be uint8 [DetectCipServiceParse:detect-cipservice.c:116]
Debug: detect-cipservice: Returning pointer (nil) of type DetectENIP ... << [DetectCipServiceParse:detect-cipservice.c:150]
FAILED
==== TEST RESULTS ====
PASSED: 0
FAILED: 1
======================```

0xEniola · 2024-02-18T08:57:21Z

I feel I also need to change the error message to incorrect input format; token "???" can not be converted to uint8 instead.

I'm also wondering why sscanf was not made to be at the beginning of the while case to be sure the input can be converted to uint8 first before performing the other checks.

jufajardini · 2024-02-21T13:56:50Z

I feel I also need to change the error message to incorrect input format; token "???" can not be converted to uint8 instead.

Minor suggestion: could instead of can not

I'm also wondering why sscanf was not made to be at the beginning of the while case to be sure the input can be converted to uint8 first before performing the other checks.

I'm guessing that this might be to ensure the input is a list of integers comma-separated. But not sure.

0xEniola · 2024-02-21T14:06:07Z

I feel I also need to change the error message to incorrect input format; token "???" can not be converted to uint8 instead.

Minor suggestion: could instead of can not

I'm also wondering why sscanf was not made to be at the beginning of the while case to be sure the input can be converted to uint8 first before performing the other checks.

I'm guessing that this might be to ensure the input is a list of integers comma-separated. But not sure.

Yes, could will be better.
Thank you for the sugestion.

I tried comma seperated inputs, and it did not work though.
So, I really do not know.

jufajardini · 2024-02-21T15:15:34Z

I feel I also need to change the error message to incorrect input format; token "???" can not be converted to uint8 instead.

Minor suggestion: could instead of can not

I'm also wondering why sscanf was not made to be at the beginning of the while case to be sure the input can be converted to uint8 first before performing the other checks.

I'm guessing that this might be to ensure the input is a list of integers comma-separated. But not sure.

I feel I also need to change the error message to incorrect input format; token "???" can not be converted to uint8 instead.

Minor suggestion: could instead of can not

I'm also wondering why sscanf was not made to be at the beginning of the while case to be sure the input can be converted to uint8 first before performing the other checks.

I'm guessing that this might be to ensure the input is a list of integers comma-separated. But not sure.

Yes, could will be better. Thank you for the sugestion.

I tried comma seperated inputs, and it did not work though. So, I really do not know.

Can you elaborate on how did you try that?

0xEniola · 2024-02-21T16:25:16Z

I feel I also need to change the error message to incorrect input format; token "???" can not be converted to uint8 instead.

Minor suggestion: could instead of can not

I'm also wondering why sscanf was not made to be at the beginning of the while case to be sure the input can be converted to uint8 first before performing the other checks.

I'm guessing that this might be to ensure the input is a list of integers comma-separated. But not sure.

I feel I also need to change the error message to incorrect input format; token "???" can not be converted to uint8 instead.

Minor suggestion: could instead of can not

I'm also wondering why sscanf was not made to be at the beginning of the while case to be sure the input can be converted to uint8 first before performing the other checks.

I'm guessing that this might be to ensure the input is a list of integers comma-separated. But not sure.

Yes, could will be better. Thank you for the sugestion.
I tried comma seperated inputs, and it did not work though. So, I really do not know.

Can you elaborate on how did you try that?

I did it via the unittests, but I guess I did it wrong before; I tried it again now, and it works.

victorjulien · 2024-02-26T14:28:45Z

src/detect-cipservice.c

        }

-        sscanf(token, "%2" SCNu8, &var);
+        if (sscanf(token, "%2" SCNu8, &var) != 1) {


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

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.

Indeed thanks for the analysis Victor.

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

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

And do an SV test.

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

I meant on the Suricata patch

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

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

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

victorjulien

see inline discussion

catenacyber · 2024-05-14T13:17:33Z

Stale PR
Proposing to use #10901 which will fix it

0xEniola requested a review from victorjulien as a code owner February 15, 2024 10:47

jufajardini added the decision-required Waiting on deliberation from the team label Feb 21, 2024

victorjulien reviewed Feb 26, 2024

View reviewed changes

victorjulien requested changes Mar 9, 2024

View reviewed changes

catenacyber closed this May 14, 2024

Comments

Conversation

0xEniola commented Feb 15, 2024

Uh oh!

victorjulien commented Feb 15, 2024

Uh oh!

0xEniola commented Feb 15, 2024

Uh oh!

0xEniola commented Feb 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xEniola commented Feb 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jufajardini commented Feb 21, 2024

Uh oh!

0xEniola commented Feb 21, 2024

Uh oh!

jufajardini commented Feb 21, 2024

Uh oh!

0xEniola commented Feb 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

victorjulien left a comment

Choose a reason for hiding this comment

Uh oh!

catenacyber commented May 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

0xEniola commented Feb 18, 2024 •

edited

Loading

0xEniola commented Feb 18, 2024 •

edited

Loading