Calling base::EscapeQueryParamValue() with untrusted inputs

[Caleb Raitto]

Is it safe to call base::EscapeQueryParamValue() in the browser process with untrusted inputs? 

I noticed that it is not listed in the rule of 2 safe types, but there is code that seems to call it in the browser process with untrusted input (for instance the "Copy link to highlight" selection right click menu option seems to, IIUC). 

I'm working on some code that requires a url::Origin to be URL-encoded -- we don't trust the renderer process to give us the correct origin. I'm also reviewing some code that might need to encode a URL component as well in the browser process.

Unlike JSON, where we have a decoder service, I don't think there's an easy way to perform URL component encoding in a utility process?

-Caleb

[Caleb Raitto]

Hi, friendly ping?

Thanks, 

-Caleb

[Caleb Raitto]

Friendly ping? Is there another mailing list I should use for this question? 

Thanks, 

-Caleb

[Emily Stark]

Sorry for the delay. (Sounds like there was possibly a miscommunication/misunderstanding; we don't have a rotation set up to monitor this list, though we do for secu...@chromium.org.)

Not my area but I'd be inclined to say this would be okay, based on the existing usage and the fact that it's a fairly short simple function; maybe you'd be willing to add a fuzzer for it for a safety belt? Hopefully if I'm wrong someone else will chime in and disagree. :)

--
To unsubscribe from this group and stop receiving emails from it, send an email to security-dev...@chromium.org.

[Emily Stark]

On Thu, Sep 15, 2022 at 4:06 PM Caleb Raitto <cara...@google.com> wrote:

Adding a fuzzer SGTM -- I think it should be similar to the existing GURL fuzzer. I can create a strawman CL, but I'm not a fuzzer expert, so I'd appreciate any tips to make the fuzzer more effective.

Thanks for the info about secu...@chromium.org -- I didn't see it on https://www.chromium.org/developers/technical-discussion-groups/ (should I add it?) security-dev@'s description seemed to match this type of discussion? https://www.chromium.org/Home/chromium-security/ implies that secu...@chromium.org is for confidential discussions?

We're seeing about adding this list to our rotation: https://chromium-review.googlesource.com/c/chromium/src/+/3901458/

 

Thanks, 

-Caleb

[Caleb Raitto]

Adding a fuzzer SGTM -- I think it should be similar to the existing GURL fuzzer. I can create a strawman CL, but I'm not a fuzzer expert, so I'd appreciate any tips to make the fuzzer more effective.

Thanks for the info about secu...@chromium.org -- I didn't see it on https://www.chromium.org/developers/technical-discussion-groups/ (should I add it?) security-dev@'s description seemed to match this type of discussion? https://www.chromium.org/Home/chromium-security/ implies that secu...@chromium.org is for confidential discussions?

Thanks, 

-Caleb

On Thu, Sep 15, 2022 at 12:20 AM Emily Stark <est...@google.com> wrote:

[Caleb Raitto]

I've mailed a fuzzer for all the base/strings/escape.h functions: crrev.com/c/3925791.

-Caleb

To unsubscribe from this group and stop receiving emails from it, send an email to security-dev+unsubscribe@chromium.org.

[Caleb Raitto]

dcheng@ (since you expressed concerns in Slack) -- would you be OK using base::EscapeQueryParamValue() in the browser process if we added the fuzzer in crrev.com/c/3925791? Or do you think we need a utility process? (I can't use the renderer for my use-case since it's not trusted to give the right answer). Or is there someone else who should weigh-in on this?

The code for that function is fairly simple, only ~20 lines. I didn't see any obvious problems myself, other than using `unsigned int` for the loop variable rather than `size_t` (IIUC, this could lead to an infinite loop if the input string is > 4GiB, but can strings that large even be passed over Mojo?)

Thanks, 

-Caleb