trifectatechfoundation / sudo-rs

A memory safe implementation of sudo and su.
Other
2.87k stars 76 forks source link

fix: off-by-one in syslog.rs #810

Closed CertainLach closed 9 months ago

CertainLach commented 9 months ago

Fixes: #809

If there is no whitespace in the string, then LIMIT + ascii_utf8_len + DOTDOTDOT_END.len() + NULL_BYTE exceeds BUFSZ = LIMIT + DOTDOTDOT_END.len() + NULL_BYTE;

I'm not sure what's the original purpose of ascii_utf8_len was, to move whitespace to the left part of the buffer? Shouldn't it be dropped then, as DOTDOTDOT_START / DOTDOTDOT_END already include space at end / at start?

I may restore this behavior, if this is preferred.

                mid = message[..mid]
                    .rfind(|c: char| c.is_ascii_whitespace())
                    .map(|p| p + ascii_utf8_len)
                    .unwrap_or(mid)
                    .max(mid)
codecov[bot] commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (5b25cb0) 57.30% compared to head (6c6b70c) 57.32%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #810 +/- ## ========================================== + Coverage 57.30% 57.32% +0.02% ========================================== Files 74 74 Lines 10497 10505 +8 ========================================== + Hits 6015 6022 +7 - Misses 4482 4483 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

squell commented 9 months ago

This fix looks reasonable to me, but the introduction of ascii_utf8_len does seem very purposeful (but also suspect, especially given that with this alteration the code passes through al the test cases with flying colours, which means it's not doing anything that we explicitly test for).

squell commented 9 months ago

@oneElectron

oneElectron commented 9 months ago

I originally wrote this code. The reason for the ascii_utf_len variable was because a precious iteration of the code required it, something which i forgot to remove later.

However, adding one to the end index of the syslog message was intended to move the whitespace from the start of the next syslog message to the end of the current message, a quick hack to hide the whitespace. I guess I didn't think about this edge case, so thank you for pointing out my mistake.

The proper method of doing this would be to leave out the whitespace entirely, and a simpler method would be to keep the +1 on the index of the last character in the buffer, but to subtract one from the limit.

Edit: The last solution would have the drawback of sometimes wasting space by pushing a word onto the next buffer, when it could habe fit on the current buffer.

squell commented 9 months ago

Thanks for clarifying. I think we can merge this as is (as a quick PR that just fixes a bug & introduces a regression test for it); the remaining issue is then what the proper of presenting long lines in the syslog should be.

I understand that the above suggestion is basically to keep the + 1 but to say

 let left = &message[..mid-1];
 let right = &message[mid..];

But I'm getting a "too hacky" sense (trying to fix a problem by introducing a subtle tweak to a formule here and there); probably it's best to just differentiate clearly between the two cases: either there is some whitespace where we can make a nice break, trim whitespace, etc., or there is none and we just have to cut the message at the limit.

Either of you feel free to open a new issue for that and shoot in a PR!

squell commented 9 months ago

Also, note that this is the kind of memory safety bug that would have gone unnoticed in C (until years down the line someone would have found a way to abuse it); so this is a very nice issue to have uncovered. Thanks both of you!