Fixing kernel commandline parameter validation #148

ita93 · 2023-05-14T17:02:57Z

According to linux kernel document, the value of command line parameter can contains spaces (with double quotes) or equals.

Summary of the PR

Please summarize here why the changes in this PR are needed.

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

All commits in this PR are signed (with git commit -s), and the commit
message has max 60 characters for the summary and max 75 characters for each
description line.
All added/changed functionality has a corresponding unit/integration
test.
All added/changed public-facing functionality has entries in the "Upcoming
Release" section of CHANGELOG.md (if no such section exists, please create one).
Any newly added unsafe code is properly documented.

roypat · 2023-05-15T09:31:28Z

Heyo, thanks for the contribution. Just a few questions, mainly for my own understanding since I couldn't find the answer in the linux docs >.>

What would happen if the value passed looks like "hello" world", e.g. with a spurious quote in the middle? Do we need to also error out in that case? Also, are single quotes allowed for the purpose of preserving spaces?

From a usability perspective, would it make sense to instead detect if the value the user passes us contains spaces, and add the quotes for them (with maybe some escaping logic for quotes already in the string, although I'm not sure the linux command line allows for escaping them).

ita93 · 2023-05-16T06:51:25Z

Heyo, thanks for the contribution. Just a few questions, mainly for my own understanding since I couldn't find the answer in the linux docs >.>

What would happen if the value passed looks like "hello" world", e.g. with a spurious quote in the middle? Do we need to also error out in that case? Also, are single quotes allowed for the purpose of preserving spaces?

Only double quotes are allowed. You can't escape " in the value, your example string is invalid.

From a usability perspective, would it make sense to instead detect if the value the user passes us contains spaces, and add the quotes for them (with maybe some escaping logic for quotes already in the string, although I'm not sure the linux command line allows for escaping them).

Maybe a good idea.

roypat · 2023-05-17T09:06:44Z

Heyo, thanks for the contribution. Just a few questions, mainly for my own understanding since I couldn't find the answer in the linux docs >.>
What would happen if the value passed looks like "hello" world", e.g. with a spurious quote in the middle? Do we need to also error out in that case? Also, are single quotes allowed for the purpose of preserving spaces?

Only double quotes are allowed. You can't escape " in the value, your example string is invalid.

I see, thanks for explaining! In that case, we should also validate that no quotes are contained in the middle of values, e.g. if value[1..value.len() - 1].contains('"') {...}

From a usability perspective, would it make sense to instead detect if the value the user passes us contains spaces, and add the quotes for them (with maybe some escaping logic for quotes already in the string, although I'm not sure the linux command line allows for escaping them).

Maybe a good idea.

If we don't have to deal with any complex escaping logic, I think we should do this. Unless I'm missing something, it'd just be

reject strings that contain quotes anywhere but the beginning or end
reject strings with unbalanced quotes
check if string contains spaces.
3.1. If it does, and it is unquoted, add quotes, otherwise leaves as is

What do others think?

andreitraistaru · 2023-05-18T09:46:21Z

Heyo, thanks for the contribution. Just a few questions, mainly for my own understanding since I couldn't find the answer in the linux docs >.>
What would happen if the value passed looks like "hello" world", e.g. with a spurious quote in the middle? Do we need to also error out in that case? Also, are single quotes allowed for the purpose of preserving spaces?

Only double quotes are allowed. You can't escape " in the value, your example string is invalid.

I see, thanks for explaining! In that case, we should also validate that no quotes are contained in the middle of values, e.g. if value[1..value.len() - 1].contains('"') {...}

From a usability perspective, would it make sense to instead detect if the value the user passes us contains spaces, and add the quotes for them (with maybe some escaping logic for quotes already in the string, although I'm not sure the linux command line allows for escaping them).

Maybe a good idea.

If we don't have to deal with any complex escaping logic, I think we should do this. Unless I'm missing something, it'd just be
1. reject strings that contain quotes anywhere but the beginning or end

2. reject strings with unbalanced quotes

3. check if string contains spaces.
   3.1. If it does, and it is unquoted, add quotes, otherwise leaves as is
What do others think?

Thanks @ita93 and @roypat for getting involved into this!

I do agree with the first 2 points (for point 2, we even have a function here that could help us with the implementation).
For point 3, I would agree to do the checking but I'm not quite sure if it would be the right approach to alter the value of the parameter in any way. I mean, if we get a wrong value for a kernel param, I do not consider the Cmdline's responsibility to correct it and store in the Cmdline as it should. For point 3, I would still reject the insertion and that's it. What do you think?

Also, what happens if we receive a parameter's value like "foo1 bar1" "foo2 bar3"? Should we also reject such a string (the case when we get nested double quotes that are balanced)? Not sure from the linux docs, but to me it looks like an invalid value as well...

ita93 · 2023-05-26T15:19:08Z

Thanks @ita93 and @roypat for getting involved into this!

I do agree with the first 2 points (for point 2, we even have a function here that could help us with the implementation). For point 3, I would agree to do the checking but I'm not quite sure if it would be the right approach to alter the value of the parameter in any way. I mean, if we get a wrong value for a kernel param, I do not consider the Cmdline's responsibility to correct it and store in the Cmdline as it should. For point 3, I would still reject the insertion and that's it. What do you think?

I think that we should follow the purpose of the original function - validating the input, that is all.

Also, what happens if we receive a parameter's value like "foo1 bar1" "foo2 bar3"? Should we also reject such a string (the case when we get nested double quotes that are balanced)? Not sure from the linux docs, but to me it looks like an invalid value as well...

As my understanding, this input may still generate a valid kernel commandline, however, the "foo2 bar3" won't belong to the current key. IMO, we should not accept this kind of input as a value of key=value pair.
We should enforce the value to have two double quotes, one at the start and one at the end of the string if there are any spaces, that is what the kernel expects

andreeaflorescu · 2023-09-05T09:00:40Z

There is no activity on this PR for some time, @ita93 are you still interested in getting this merged? If not, can you please close the PR?

ita93 · 2024-01-24T10:00:45Z

There is no activity on this PR for some time, @ita93 are you still interested in getting this merged? If not, can you please close the PR?

Sorry for the late reply, I'm still interested in this one. I will try posting it in Slack to ask for a reviewing

roypat · 2024-11-14T13:17:52Z

(replying now since I saw you rebased this recently, sorry for dropping the ball last year!)

Going over the old discussion from last year, I think the only thing left here is to handle the "hello "world" and "foo1 bar1" "foo2 bar3" examples above, and also reject them, right? Also, this should probably get a Changelog entry

rbradford · 2024-11-14T14:45:36Z

You should also rebase your commits to drop the merges.

roypat · 2024-11-14T14:48:58Z

You should also rebase your commits to drop the merges.

I think they're all merge commits created by the github UI (they're all empty), so they'll be dropped if we merge by rebasing

ita93 · 2024-11-22T06:26:10Z

Going over the old discussion from last year, I think the only thing left here is to handle the "hello "world" and "foo1 bar1" "foo2 bar3" examples above, and also reject them, right? Also, this should probably get a Changelog entry

Hi, I've added a changelog entry in the newest commit.
In summary, the rules will be:

reject strings that contain quotes anywhere but the beginning or end
reject strings that contain spaces but are not quoted.

CHANGELOG.md

TimePrinciple

Some thoughts :)

TimePrinciple · 2024-11-22T07:53:56Z

src/cmdline/mod.rs

+fn is_quoted(s: &str) -> bool {
+    if let Some(first_char) = s.chars().next() {
+        if let Some(last_char) = s.chars().last() {
+            return first_char == '"' && last_char == '"';
+        }
+    }
+    false
+}


IMHO this implementation would accept input with only a '"' character, should we check if next() and last() actually pointing different location 🤔, or else we check the length of s which should be at least 2 characters long

Hi, I've updated this function as per your suggestion. Can you help me to review it again, please?

roypat

Thanks! Could you squash the last commit such that we don't have commits that modify code introduces in earlier commits though?

According to linux kernel document, the value of command line parameter can contains spaces (with double quotes) or equals. This change will also reject the parameter value that contains quotes in the middle. Signed-off-by: Nguyen Dinh Phi <[email protected]>

rbradford

Rubber stamping after @roypat's review.

ita93 requested review from alxiord, sameo and rbradford as code owners May 14, 2023 17:02

ita93 force-pushed the fix/value_validation branch from e2610af to 9db1c32 Compare June 4, 2023 03:06

ita93 requested review from roypat and andreitraistaru as code owners June 4, 2023 03:06

ita93 force-pushed the fix/value_validation branch 2 times, most recently from c085ab6 to db4d7a4 Compare June 4, 2023 05:26

ita93 requested a review from JonathanWoollett-Light as a code owner September 6, 2023 03:04

ita93 requested a review from ShadowCurse as a code owner November 6, 2024 16:08

ita93 force-pushed the fix/value_validation branch from 1afbee4 to cd4ee10 Compare November 21, 2024 13:04

roypat reviewed Nov 22, 2024

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

TimePrinciple reviewed Nov 22, 2024

View reviewed changes

ita93 force-pushed the fix/value_validation branch 3 times, most recently from 8f71b30 to ac22037 Compare November 26, 2024 06:47

roypat previously approved these changes Nov 26, 2024

View reviewed changes

ita93 force-pushed the fix/value_validation branch from ac22037 to 2f1d19a Compare November 27, 2024 08:41

ita93 dismissed roypat’s stale review via 8a32810 November 27, 2024 08:47

ita93 force-pushed the fix/value_validation branch 2 times, most recently from f01fb8a to 772ee95 Compare November 27, 2024 09:29

ita93 force-pushed the fix/value_validation branch from 772ee95 to 2d5e73b Compare November 27, 2024 09:32

roypat approved these changes Nov 27, 2024

View reviewed changes

rbradford approved these changes Nov 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing kernel commandline parameter validation #148

Fixing kernel commandline parameter validation #148

ita93 commented May 14, 2023

roypat commented May 15, 2023

ita93 commented May 16, 2023 •

edited

Loading

roypat commented May 17, 2023

andreitraistaru commented May 18, 2023

ita93 commented May 26, 2023

andreeaflorescu commented Sep 5, 2023

ita93 commented Jan 24, 2024

roypat commented Nov 14, 2024

rbradford commented Nov 14, 2024

roypat commented Nov 14, 2024

ita93 commented Nov 22, 2024 •

edited

Loading

TimePrinciple left a comment

TimePrinciple Nov 22, 2024

ita93 Nov 27, 2024

roypat left a comment

rbradford left a comment

Fixing kernel commandline parameter validation #148

Are you sure you want to change the base?

Fixing kernel commandline parameter validation #148

Conversation

ita93 commented May 14, 2023

Summary of the PR

Requirements

roypat commented May 15, 2023

ita93 commented May 16, 2023 • edited Loading

roypat commented May 17, 2023

andreitraistaru commented May 18, 2023

ita93 commented May 26, 2023

andreeaflorescu commented Sep 5, 2023

ita93 commented Jan 24, 2024

roypat commented Nov 14, 2024

rbradford commented Nov 14, 2024

roypat commented Nov 14, 2024

ita93 commented Nov 22, 2024 • edited Loading

TimePrinciple left a comment

Choose a reason for hiding this comment

TimePrinciple Nov 22, 2024

Choose a reason for hiding this comment

ita93 Nov 27, 2024

Choose a reason for hiding this comment

roypat left a comment

Choose a reason for hiding this comment

rbradford left a comment

Choose a reason for hiding this comment

ita93 commented May 16, 2023 •

edited

Loading

ita93 commented Nov 22, 2024 •

edited

Loading