Possible glob incompatibility with original sudo

[tertsdiepraam]

Describe the bug

Reading your recent blog post regarding dependencies, I learned that sudo-rs depends on glob. I figured that you might have a similar problem as uutils with this crate. In particular, we once opened this issue: rust-lang/glob#116.

To recap that issue: glob only allows [!...] for negation of character classes, but not [^...]. The standard fnmatch and glob functions usually do allow ^ to be used, including the implementation by sudo, even though it does not seem to be documented.

I checked src/sudoers/tokens.rs and could not find a mitigation for this there. There also aren't any occurrences of '^' in the code base according to GitHub search, which I would expect to see if you implemented a workaround for this issue.

I'm not sure how big of an issue this is, but it's probably at least an incompatibility that should be documented. I'm also not really sure how to create a test case for this, but if you can point me to documentation for that, I'd be happy to try to create one.

[squell]

Here's my take, also taking into account the criteria for inclusion.

• This should (for the time being) definitely be documented in the appropriate README section. We're more than willing to accept PR's for our README as well. 😏
• A "workaround" in sudo-rs for this is not going to happen. That's non-trivial to get right (see @ndmitchell's comment in rust-lang/glob/issues/116).
• We could remove the glob crate as a dependency and use standard glob/fnmatch. The downside of that is that while our dependency count goes even further down, you're still depending on someone's glob implementation, except that we won't know which one, and we would be replacing Rust code with C code, with the concomitant risks. That, at least, were the reasons to keep it as a dependency.

So I would say, if this needs addressing, it needs addressing for everybody, and the glob crate is the place to do it. I do think that supporting both ! and ^ is perfectly reasonable. Perhaps the easiest way forward is to simply give them a PR to review?

[tertsdiepraam]

I agree with your assessment and I'll try to put up a PR for glob and link this issue in addition to the one in uutils.

[tertsdiepraam]

There seem to be some other subtle differences documented by sudo:

1. A bracket expression starting with an unquoted '^' character CONTINUES to specify a non-matching list;
2. an explicit period '.' in a bracket expression matching list, e.g. "[.abc]" does NOT match a leading period in a filename;
3. a left-square-bracket '[' which does not introduce a valid bracket expression is treated as an ordinary character;
4. a differing number of consecutive slashes within pattern and string will NOT match;
5. a trailing '\' in FNM_ESCAPE mode is treated as an ordinary '\' character.

Source: https://github.com/sudo-project/sudo/blob/b6175b78ad1c4c9535cad48cb76addf53352a28f/lib/util/fnmatch.c#L61-L70

Some of these will not be very important. The third point for example seems fine if the glob crate is more restrictive and just yields an error. That's incompatible, but it can't be misused accidentally. The fifth one also seems less important because it seems like glob doesn't do escaping. I think these are some tests for cases 2,3 and 4:

use glob::Pattern;

fn main() {
    let p = Pattern::new("[.abc]foo.txt").unwrap();
    let r = p.matches(".foo.txt");
    println!("Case 2: Class should not match leading `.`: {}", !r);
    
    let p = Pattern::new("[.txt");
    println!("Case 3: Unmatched [ is allowed: {}", p.is_ok());
    
    let p = Pattern::new("foo/bar.txt").unwrap();
    let r = p.matches("foo//bar.txt");
    println!("Case 4: Different number of slashes should not match: {}", !r);
}

This prints:

Case 2: Class should not match leading `.`: false
Case 3: Unmatched [ is allowed: false
Case 4: Different number of slashes should not match: true

This means that glob already does case 4 correctly, but doesn't match the behaviour on 2 and 3.

Edit: glob does provide a require_literal_leading_dot option that provides the correct behaviour for the leading dot issue.

[squell]

Note: making our own glob seems like a bad idea, see #1136 (comment)

But the incompatibilities mentioned above still exists, and are not easy to fix, see also: #1043

I also noticed rust-lang/glob#141 (review).

So I think what the sane thing to do is to not "roll our own", but instead give users and option of continueing to use the glob crate (a bit more memory safe, maybe less compatible in corner cases), or use whatever the system's libc is offering, which is guaranteed to do the same thing as ogsudo. (Also, most vulnerabilities in fnmatch() probably expect the attacker to have control over the pattern, which is not an attack vector for sudo)

On last escape is of course to port ogsudo's sudo_fnmatch function to Rust. But I don't know how good that one is or how well-tested in practice--on most machines ogsudo will be configured to use libc's fnmatch, and This doesn't support UTF8 actually.