Make api::icon::is_valid_domain more robust/accurate

[zacknewman]

api::icon::is_valid_domain can be made more accurate without too much extra code. I think the only RFC that should matter is RFC 2181. While RFCs 1123 and 5891 are nice, neither one is a subset of the other; and more relevantly, browsers like Firefox are more permissive than both of those RFCs. For that reason I propose changing the logic of this function to allow whatever URL web browsers allow (sans those that are transformed into Punycode). More specifically, the following Unicode scalar values should be allowed as they are allowed by Firefox and likely other browsers:

!, $, &, ', (, ), +, ,, -, 0–9, ;, =, _, `, A–Z, a–z, {, }, ~, ..

Additionally the check on length is not strict enough. While RFC 2181 allows domain names to be as many as 255 bytes in length, that is the wire format and not the representation format that we are interested in. Specifically, the maximum length in representation format (ignoring server-specific escape characters) is 254 iff the last character is .; otherwise the max length is 253.

diff:

fn is_valid_domain(domain: &str) -> bool {
    // The sequence of Unicode scalar values we allow make up a proper subset
    // of ASCII which UTF-8 is backwards compatible with, so we can view `domain`
    // more simply as a `[u8]`.
    let mut value = domain.as_bytes();
    value = match value.last() {
        // Empty domains are not allowed.
        None => return false,
        Some(byt) => {
            if *byt == b'.' {
                // `.` is a valid domain.
                if value.len() == 1 {
                    return true;
                }
                // We allow a terminating `.`; however we remove it
                // for simpler calculation of label lengths
                // and total domain length.
                // This won't `panic` since `value.last()` returned `Some`.
                &value[..value.len() - 1]
            } else {
                value
            }
        }
    };
    // The maximum length a domain without a trailing `.` in representation format
    // is allowed to be is 253 (ignoring server-specific escape characters) per RFC 2181.
    value.len() < 254
        && value
            .into_iter()
            .try_fold(0u8, |len, byt| {
                match *byt {
                    // labels must not be empty.
                    b'.' => return if len == 0 { Err(()) } else { Ok(0) },
                    // Unicode scalar values were chosen based on what Firefox allows
                    // as of 2023-11-13T18:12+00:00.
                    b'!'
                    | b'$'
                    | b'&'..=b')'
                    | b'+'..=b'-'
                    | b'0'..=b'9'
                    | b';'
                    | b'='
                    | b'A'..=b'Z'
                    | b'_'..=b'{'
                    | b'}'..=b'~' => {}
                    _ => return Err(()),
                }
                // labels must have length less than 64.
                if len == 63 {
                    Err(())
                } else {
                    Ok(len + 1)
                }
            })
            .map_or(false, |len| {
                // Also want to only allow domains that `Url` is able to parse.
                len > 0 && Url::parse(format!("https://{domain}").as_str()).is_ok()
            })
}

[BlackDex]

I think this is a bit too much. The validation of the domain it's done here already

vaultwarden/src/api/icons.rs

Line 140 in f863ffb

if let Err(parse_error) = url::Url::parse(format!("https://{domain}").as_str()) {

That has all the validation tooling already.
We only do some extra checks to prevent traversing for example.

Is there a specific reason for this? Are running into an issue?

[zacknewman]

That part of the check is also handled with this diff, however the rest of the if block in the current code performs additional checks that allow too much (i.e., domains that are actually too long) and not enough based on legitimate URLs.

The reason is to be more correct. I disable icon fetching, so no there is not an issue I am personally facing; however I can very easily construct an icon with a URL that fails the current check. Mind you a lot of this diff is similar except it explicitly checks the ASCII characters as opposed to relying on functions in core/std that do a similar thing (e.g., char::is_alphanumeric). One benefit of doing the explicit pass is you can also verify label lengths with almost no additional cost.

You can also reduce the amount of code if you don't mind redundant checks that will cause a minor hit in performance (almost certainly an imperceptible amount). This diff attempts to be as "fast" as possible by performing a single pass plus of course one more implicit pass from Url::parse.

[zacknewman]

Performance is in no way the reason for this diff; however since you mentioned "We only do some extra checks to prevent traversing for example", I decided to parse ≈ 300K domains; and this diff is ≈ 30% faster than the current code. So in addition to robustness, I suppose you also get "performance".

To add to this, if one is OK removing Url::parse altogether; then it's > 90% faster and that's for valid domains. The performance gain may even be more pronounced for invalid domains since a String allocation happens every time in the current code. I don't believe in micro-optimizations though, so the purpose of this diff is purely about correctness/robustness invariant of whether or not I am personally affected.

[BlackDex]

Thx, I'll have a look at it too see if this also works for all cases i have locally. Performance is nice indeed, but security and validity are more of a trigger for me.

[zacknewman]

I use very similar code in another project I maintain, so please provide some examples where this fails as I'd like to fix the code. The code has not undergone formal verification, but it has passed fuzzing and unit tests of over 2^32 inputs. If it were me, I would personally remove Url::parse altogether; but that's your call. Also you can reduce the allowed ASCII characters if for some reason you insist on not allowing them. As I said, those ASCII characters are accepted in domain names when making requests in Firefox; so that's why I also allow them.

[BlackDex]

Well, we don't allow : and =, also & is actually used in a URL not in a domain. Domains should actually follow rfc1035, which doesn't allow any of these special characters.

Since Bitwarden only sends domains and strips ports, paths, arguments, protocol etc, we really only should receive a FQDN. While a FQDN does not have a rfc i believe, i think it starts with the rfc1035 as a base.

Any other character could potentially be a security risk i think.

[zacknewman]

: is not allowed in this code. RFC 1035 is one possible definition of a "domain name". RFC 1181 and RFC 5891 are also possible definitions as is RFC 2181 which is the spec for DNS as a whole. It is perfectly legal for me to publish AAAA, A, etc. records that only conform to RFC 2181; and one can use that as the definition of "domain name". My implementation is more permissive than 1035, 1181, and 5891; but obviously a lot more conservative than 2181. As I said, the characters were chosen since that's what browsers allow.

Please don't call things security risks without evidence especially since the current code violates every RFC mentioned above (including 1035 and even 2181 (i.e., the DNS spec)). The current code allows WAY more domains than what my code allows and what RFCs 1035, 1181, and 5891 allow. My code only allows 15 ASCII characters that the current code does not: !, $, &, ', (, ), +, ,, -, ;, =, `, {, }, and ~ (again, because browsers allow those characters in the domain (not just other parts of the URI)).

Current code allows for 132,948 Unicode scalar values that this code does not. So if anything, the current code is more a "security risk" than mine; but I don't like using such language without explicitly defining a threat model a priori.

You can very easily edit the proposed code to disallow characters you don't what based on whatever definition of "domain" you want to use; however as stated above and what the below shows, if you plan to support WAY more domains than what you are claiming you want to allow (including RFC 1035) and more specifically want to allow non-ASCII characters, then the proposed code should be nixed since it is far more closely aligned with the aforementioned RFCs (including RFC 1035).

Code that compares what Unicode scalar values are allowed (note this code also shows the domain . is allowed in the proposed code but not the current, so the counts are off by one to what was quoted above):

fn main() {
    let mut slice = [0u8; 4];
    let mut valid_new_not_cur_counter = 0;
    let mut valid_cur_not_new_counter = 0;
    for i in 0..=u32::MAX {
        char::from_u32(i).map(|uni| {
            let dom = uni.encode_utf8(&mut slice);
            if is_valid_domain_new(dom) {
                if !is_valid_domain_cur(dom) {
                    valid_new_not_cur_counter += 1;
                    println!("ASCII character that is valid in proposed code but not current code: {dom}");
                }
            } else if is_valid_domain_cur(dom) {
                valid_cur_not_new_counter += 1;
                println!("Unicode code point that is valid in current code but not proposed code: {i}");
            }
        });
    }
    println!("Count of 'characters' that are valid in proposed code but not current: {valid_new_not_cur_counter}");
    println!(
        "Count of 'characters' that are valid in current code but not proposed: {valid_cur_not_new_counter}"
    );
}
fn is_valid_domain_cur(domain: &str) -> bool {
    const ALLOWED_CHARS: &str = "_-.";

    // If parsing the domain fails using Url, it will not work with reqwest.
    if let Err(_) = url::Url::parse(format!("https://{domain}").as_str()) {
        return false;
    } else if domain.is_empty()
        || domain.contains("..")
        || domain.starts_with('.')
        || domain.starts_with('-')
        || domain.ends_with('-')
    {
        return false;
    } else if domain.len() > 255 {
        return false;
    }

    for c in domain.chars() {
        if !c.is_alphanumeric() && !ALLOWED_CHARS.contains(c) {
            return false;
        }
    }

    true
}
fn is_valid_domain_new(domain: &str) -> bool {
    // The sequence of Unicode scalar values we allow make up a proper subset
    // of ASCII which UTF-8 is backwards compatible with, so we can view `domain`
    // more simply as a `[u8]`.
    let mut value = domain.as_bytes();
    value = match value.last() {
        // Empty domains are not allowed.
        None => return false,
        Some(byt) => {
            // We allow a terminating `.`; however we remove it
            // for simpler calculation of label lengths
            // and total domain length.
            if *byt == b'.' {
                // `.` is a valid domain.
                if value.len() == 1 {
                    return true;
                }
                // This won't `panic` since `value.last()` returned `Some`.
                &value[..value.len() - 1]
            } else {
                value
            }
        }
    };
    // The maximum length a domain without a trailing `.` in representation format
    // is allowed to be per RFC 2181 is 253.
    value.len() < 254
        && value
            .into_iter()
            .try_fold(0u8, |len, byt| {
                match *byt {
                    // labels must not be empty.
                    b'.' => return if len == 0 { Err(()) } else { Ok(0) },
                    // Unicode scalar values were chosen based on what Firefox allows
                    // as of 2023-11-13T18:12+00:00.
                    b'!'
                    | b'$'
                    | b'&'..=b')'
                    | b'+'..=b'-'
                    | b'0'..=b'9'
                    | b';'
                    | b'='
                    | b'A'..=b'Z'
                    | b'_'..=b'{'
                    | b'}'..=b'~' => {}
                    _ => return Err(()),
                }
                // labels must have length less than 64.
                if len == 63 {
                    Err(())
                } else {
                    Ok(len + 1)
                }
            })
            .map_or(false, |len| len > 0)
}

[BlackDex]

With a risk i mean with your code can then provide my.domain.tld&my_arg=value which would be valid i think (not tested), which if we would use that as is could trigger actions not wanted.

But as said, i have not tested it, but i will check it out.
Thanks for the more detailed example to show the difference.

[zacknewman]

Yes, the proposed code says that's valid as does Firefox and Url. You can remove = or any other ASCII characters if you want.

fn main() {
    let input = "my.domain.tld&my_arg=value";
    // This does _not_ `panic`.
    assert!(
        url::Url::parse(format!("https://{input}").as_str()).map_or(false, |url| url
            .domain()
            .map_or(false, |domain| domain == input))
    );
}

[zacknewman]

If the proposed code is rejected since you want to allow non-ASCII characters, then the current code still should be amended since it even violates RFC 2181 which is obviously the most permissive definition of "domain" since it's the DNS spec. Specifically, the current code currently allows domains with labels of lengths longer than 63 and also allows domains that are longer than the maximum length possible in representation format: 253 when the last character is not . or 254 when the last character is ..

fn main() {
    let illegal_label = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
    // This does _not_ `panic` but should since RFC 2181 max label lengths are violated.
    assert!(illegal_label.len() == 64 && is_valid_domain_cur(illegal_label));
    let mut illegal_domain = String::with_capacity(255);
    let longest_legal_label = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
    illegal_domain.push_str(longest_legal_label);
    illegal_domain.push('.');
    illegal_domain.push_str(longest_legal_label);
    illegal_domain.push('.');
    illegal_domain.push_str(longest_legal_label);
    illegal_domain.push('.');
    illegal_domain.push_str(longest_legal_label);
    // This does _not_ `panic` but should since RFC 2181 max length of a domain in
    //representation format is violated.
    assert!(longest_legal_label.len() == 63 && illegal_domain.len() == 255 && is_valid_domain_cur(illegal_domain.as_str()));
}

[zacknewman]

So then remove the function completely. If you are going to use it, then it should at least be correct; or at the very least, only rely on Url::parse and define the correctness of the function as Url::parse. It seems bizarre to have additional checks that are not compliant to any reasonable definition.

To your points:

1. The current code overrides what the Bitwarden clients say. So if a Bitwarden client says my.domain.tld&my_arg=value is a domain (not just URL), then why is Vaultwarden overriding that?
2. Read 1. Not only is this too permissive than what it claims, but it also may reject requests that it shouldn't if the Bitwarden client says it's valid.
3. None of this has anything to do with any part of a URL that is not the domain. Url says it's a valid domain (not just URL). Browsers say it's a valid domain (not just URL). Most important of all, the Bitwarden client itself might say so.

[BlackDex]

Then you should read the function better and look at what the purpose is of the function.

Again, thanks for all the comments, and I'll see if we can improve this function 😃

[zacknewman]

You keep contradicting yourself. First you claimed the current function attempts to avoid "traversing". This clearly is not true because Url::parse is always called in addition to the String allocation that always happens. You then claimed "security and validity" are what's important. I showed you the function is not valid. Your first attempt at justifying "security" was also easily refuted since the RFC you mentioned was never adhered to and your claim "Any other character could potentially be a security risk i think" is again not true due to how many characters are allowed. You then cite a domain (not just URL) that you claim is "dangerous" without actually explaining how it's dangerous which is security theater if you ask me especially since your initial attempt at a "security" argument was wrong. Then you keep talking about the non-domain part of the URL which at no point has ever been mentioned. Then finally you claim it doesn't matter anyway since the Bitwarden clients accepted it but still insist the function provides value. I'll just close this discussion.

[BlackDex]

You seem to be a good developer in the strictness of the sense.
Nothing wrong with that. But, we check same basic stuff, as mentioned in the comments.

Also, we check if a domain isn't starting with dots which could cause reverse path traversing. And some other checks, so stating this function should just be removed and only use the Url::parse which of course is not checking those points.

It could be that we can make this function better, but we should not forget the reasons of these checks. Maybe a bit more comment there might help in clarifying what it does for which reason 😃

[zacknewman]

When you originally said "traversing", I mistook that as "string traversal" not directory traversal which is why I mentioned performance at all. Now it makes sense what you meant by that.

[BlackDex]

Calling the parse function and also checking if it starts with two dots doesn't rule out the check. If parse fails or the custom char start/end/contains check is valid it will return false.

Domains aren't always punycode and thus we need to allow valid Unicode characters which are allowed to be used which is exactly what is_alphanumeric does check.

I never said that we adhere to the rfc, as you can see in our code. I only mentioned it because that doesn't allow the special characters.

I also mentioned why allowing & and = could cause potential issues, as those are seen as arguments in an url, and if this were passed on as a call it could load a different page or trigger an action on that endpoint.

I also didn't say it doesn't matter anyway, i mentioned that most are coming from the clientsb from entries added in a normal way. But people can still call that endpoint manually or enter an invalid domain into the url field. We still should try to filter those out.

Also, i mentioned several times i would take a look at this and see if we can improve. That doesn't mean today or maybe not even tomorrow. So closing this is in my opinion unnecessary and not very productive way of working. Especially when i mention why we do some checks and why we do not want or need specific characters to be allowed for a domain.

Anyways, I'm still glad you mentioned this, any fresh look to things done longer time ago is good. Thx for that.