Fixing kernel commandline parameter validation

[ita93]

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]

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]

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]

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?

[andreitraistaru]

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]

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]

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]

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