Skip to content

Commit 4337071

Browse files
authored
Fix username escaping (#1054)
2 parents c8d958a + 2c1cb23 commit 4337071

File tree

5 files changed

+92
-27
lines changed

5 files changed

+92
-27
lines changed

src/sudoers/ast.rs

Lines changed: 34 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -289,23 +289,41 @@ impl Parse for Meta<Identifier> {
289289
/// ```
290290
impl Parse for UserSpecifier {
291291
fn parse(stream: &mut CharStream) -> Parsed<Self> {
292-
let userspec = if stream.eat_char('%') {
293-
let ctor = if stream.eat_char(':') {
294-
UserSpecifier::NonunixGroup
292+
fn parse_user(stream: &mut CharStream) -> Parsed<UserSpecifier> {
293+
let userspec = if stream.eat_char('%') {
294+
let ctor = if stream.eat_char(':') {
295+
UserSpecifier::NonunixGroup
296+
} else {
297+
UserSpecifier::Group
298+
};
299+
// in this case we must fail 'hard', since input has been consumed
300+
ctor(expect_nonterminal(stream)?)
301+
} else if stream.eat_char('+') {
302+
// TODO Netgroups
303+
unrecoverable!(stream, "netgroups are not supported yet");
295304
} else {
296-
UserSpecifier::Group
305+
// in this case we must fail 'softly', since no input has been consumed yet
306+
UserSpecifier::User(try_nonterminal(stream)?)
297307
};
298-
// in this case we must fail 'hard', since input has been consumed
299-
ctor(expect_nonterminal(stream)?)
300-
} else if stream.eat_char('+') {
301-
// TODO Netgroups
302-
unrecoverable!(stream, "netgroups are not supported yet");
303-
} else {
304-
// in this case we must fail 'softly', since no input has been consumed yet
305-
UserSpecifier::User(try_nonterminal(stream)?)
306-
};
307308

308-
make(userspec)
309+
make(userspec)
310+
}
311+
312+
// if we see a quote, first parse the quoted text as a token and then
313+
// re-parse whatever we found inside; this is a lazy solution but it works
314+
let begin_pos = stream.get_pos();
315+
if stream.eat_char('"') {
316+
let Unquoted(text, _): Unquoted<Username> = expect_nonterminal(stream)?;
317+
let result = parse_user(&mut CharStream::new(text.chars()));
318+
if result.is_err() {
319+
unrecoverable!(pos = begin_pos, stream, "invalid user")
320+
}
321+
expect_syntax('"', stream)?;
322+
323+
result
324+
} else {
325+
parse_user(stream)
326+
}
309327
}
310328
}
311329

@@ -570,7 +588,7 @@ impl Parse for Sudo {
570588
fn parse_include(stream: &mut CharStream) -> Parsed<Sudo> {
571589
fn get_path(stream: &mut CharStream, key_pos: (usize, usize)) -> Parsed<(String, Span)> {
572590
let path = if stream.eat_char('"') {
573-
let QuotedInclude(path) = expect_nonterminal(stream)?;
591+
let QuotedIncludePath(path) = expect_nonterminal(stream)?;
574592
expect_syntax('"', stream)?;
575593
path
576594
} else {
@@ -744,7 +762,7 @@ impl Parse for defaults::SettingsModifier {
744762
// Parse a text parameter
745763
let text_item = |stream: &mut CharStream| {
746764
if stream.eat_char('"') {
747-
let QuotedText(text) = expect_nonterminal(stream)?;
765+
let QuotedStringParameter(text) = expect_nonterminal(stream)?;
748766
expect_syntax('"', stream)?;
749767
make(text)
750768
} else {

src/sudoers/ast_names.rs

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -65,21 +65,24 @@ mod names {
6565
impl UserFriendly for UserSpecifier {
6666
const DESCRIPTION: &'static str = "user";
6767
}
68+
impl UserFriendly for tokens::Username {
69+
const DESCRIPTION: &'static str = "user";
70+
}
6871

6972
impl UserFriendly for tokens::Hostname {
7073
const DESCRIPTION: &'static str = "host name";
7174
}
7275

73-
impl UserFriendly for tokens::QuotedText {
76+
impl UserFriendly for tokens::QuotedStringParameter {
7477
const DESCRIPTION: &'static str = "non-empty string";
7578
}
7679

77-
impl UserFriendly for tokens::QuotedInclude {
80+
impl UserFriendly for tokens::QuotedIncludePath {
7881
const DESCRIPTION: &'static str = "non-empty string";
7982
}
8083

8184
impl UserFriendly for tokens::StringParameter {
82-
const DESCRIPTION: &'static str = tokens::QuotedText::DESCRIPTION;
85+
const DESCRIPTION: &'static str = tokens::QuotedStringParameter::DESCRIPTION;
8386
}
8487

8588
impl UserFriendly for tokens::IncludePath {
@@ -113,6 +116,10 @@ mod names {
113116
impl<T> UserFriendly for Def<T> {
114117
const DESCRIPTION: &'static str = "alias definition";
115118
}
119+
120+
impl<T: UserFriendly> UserFriendly for tokens::Unquoted<T> {
121+
const DESCRIPTION: &'static str = T::DESCRIPTION;
122+
}
116123
}
117124

118125
#[cfg(test)]

src/sudoers/basic_parser.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,6 +231,10 @@ impl<T: Token> Parse for T {
231231
Ok(c)
232232
} else if pred(ESCAPE) {
233233
Ok(ESCAPE)
234+
} else if stream.eat_char('\n') {
235+
// we've just consumed some whitespace, we should end
236+
// parsing the token but not abort it
237+
reject()
234238
} else {
235239
unrecoverable!(stream, "illegal escape sequence")
236240
}

src/sudoers/test/mod.rs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -240,11 +240,16 @@ fn permission_test() {
240240
pass!(["Runas_Alias TIME=%wheel,!!sudo","user ALL=(:TIME) ALL"], "user" => request! { user, sudo }, "vm"; "/bin/ls");
241241
pass!(["Runas_Alias TIME=%wheel,!!sudo","user ALL=(TIME) ALL"], "user" => request! { wheel, wheel }, "vm"; "/bin/ls");
242242

243-
pass!(["Runas_Alias \\"," TIME=%wheel\\",",sudo # hallo","user ALL\\","=(TIME) ALL"], "user" => request! { wheel, wheel }, "vm"; "/bin/ls");
243+
pass!(["Runas_Alias \\"," TIME=%wheel \\",",sudo # hallo","user ALL \\","=(TIME) ALL"], "user" => request! { wheel, wheel }, "vm"; "/bin/ls");
244244

245245
// test the less-intuitive "substition-like" alias mechanism
246246
FAIL!(["User_Alias FOO=!user", "ALL, FOO ALL=ALL"], "user" => root(), "vm"; "/bin/ls");
247247
pass!(["User_Alias FOO=!user", "!FOO ALL=ALL"], "user" => root(), "vm"; "/bin/ls");
248+
249+
// quoting
250+
pass!(["a\\,b ALL=ALL"], "a,b" => request! { root, root }, "server"; "/bin/foo");
251+
pass!(["\"a,b\" ALL=ALL"], "a,b" => request! { root, root }, "server"; "/bin/foo");
252+
pass!(["\"a\\b\" ALL=ALL"], "a\\b" => request! { root, root }, "server"; "/bin/foo");
248253
}
249254

250255
#[test]

src/sudoers/tokens.rs

Lines changed: 38 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,11 @@ impl Token for Username {
2323
fn accept_1st(c: char) -> bool {
2424
c != '_' && Self::accept(c)
2525
}
26+
27+
const ALLOW_ESCAPE: bool = true;
28+
fn escaped(c: char) -> bool {
29+
matches!(c, '\\' | '"' | ',' | ':' | '=' | '!' | '(' | ')')
30+
}
2631
}
2732

2833
impl Many for Username {}
@@ -275,13 +280,14 @@ impl Token for EnvVar {
275280
}
276281
}
277282

278-
pub struct QuotedText(pub String);
283+
/// A token with a very liberal inner tokenizer; compare StringParameter below
284+
pub struct QuotedStringParameter(pub String);
279285

280-
impl Token for QuotedText {
286+
impl Token for QuotedStringParameter {
281287
const MAX_LEN: usize = 1024;
282288

283289
fn construct(s: String) -> Result<Self, String> {
284-
Ok(QuotedText(s))
290+
Ok(Self(s))
285291
}
286292

287293
fn accept(c: char) -> bool {
@@ -294,15 +300,17 @@ impl Token for QuotedText {
294300
}
295301
}
296302

303+
/// Similar to QuotedStringParameter but treats backslashes differently
304+
/// Compare IncludePath below.
297305
// `@include "some/path"`
298306
// ^^^^^^^^^^^
299-
pub struct QuotedInclude(pub String);
307+
pub struct QuotedIncludePath(pub String);
300308

301-
impl Token for QuotedInclude {
309+
impl Token for QuotedIncludePath {
302310
const MAX_LEN: usize = 1024;
303311

304312
fn construct(s: String) -> Result<Self, String> {
305-
Ok(QuotedInclude(s))
313+
Ok(Self(s))
306314
}
307315

308316
fn accept(c: char) -> bool {
@@ -338,7 +346,7 @@ impl Token for IncludePath {
338346
pub struct StringParameter(pub String);
339347

340348
impl Token for StringParameter {
341-
const MAX_LEN: usize = QuotedText::MAX_LEN;
349+
const MAX_LEN: usize = QuotedStringParameter::MAX_LEN;
342350

343351
fn construct(s: String) -> Result<Self, String> {
344352
Ok(StringParameter(s))
@@ -391,3 +399,26 @@ impl Token for ChDir {
391399
matches!(c, '\\' | '"' | ' ')
392400
}
393401
}
402+
403+
/// Some tokens that support escape characters also support being surrounded by quotes to avoid escaping directly.
404+
pub struct Unquoted<T>(pub String, pub std::marker::PhantomData<T>);
405+
406+
impl<T: Token> Token for Unquoted<T> {
407+
const MAX_LEN: usize = 1024;
408+
409+
fn construct(text: String) -> Result<Self, String> {
410+
let mut quoted = String::new();
411+
for ch in text.chars() {
412+
if T::escaped(ch) {
413+
quoted.push('\\');
414+
}
415+
quoted.push(ch);
416+
}
417+
418+
Ok(Self(quoted, std::marker::PhantomData))
419+
}
420+
421+
fn accept(c: char) -> bool {
422+
c != '"' && !c.is_control()
423+
}
424+
}

0 commit comments

Comments
 (0)