Url / Process argument sanitization

[Fijo]

Going though the code, I've noticed the following thing, that left me feeling unwell about using this tool, for now.

I was following the Url, that's passed into the tool via a command line argument.
From what I can see there is no check, to ensure it doesn't contain a double quote (").

Some of the flow

1. Entering BrowserService.LaunchAsync(string rawUrl, string windowTitle)

2. Assuming there are no filters, url will end up with the same content as rawUrl.

3. Uri? uri = UriFactory.Get(url); is executed.
   My research:
   If I run Uri? uri = UriFactory.Get("https://www.example.com/path?token=\""),
   uri will NOT be null.
   Therefore execution is continued with this possibly problematic url.

4. args = Args.Format(args, uri); is executed.
   Lets say there are no tag replacements, which should be the most common case.
   In that case the url will basically end up in the args like this:
   $" \"{uri.OriginalString}\""

   uri.OriginalString should match the original value passed into the UriFactory.Get-Method earlier (based on it's name),
   so that's just the content of url again.

5. args is now passed into the following method call: process.Start(path, args).
   Within it System.Diagnostics.Process.Start(path, args) is called.

With the correct original url, it may be possible to end up with args that inject additional flags into the Process-Start-Call.
This could possibly lead to a security vulnerability.

Example:
Original Url:
https://www.example.com/path?token=" --unsafely-treat-insecure-origin-as-secure "https://www.example.com/
Should in theory result in a (bad) argument-string like:
"https://www.example.com/path?token=" --unsafely-treat-insecure-origin-as-secure "https://www.example.com/"

Therefore I suggest you add a check somewhere to ensure the url does not contain double quotes.
Maybe a backslah would also be bad at the end (I didn't dig into that, yet).

Thanks in advance!

PS.:
Thanks for your work on this neat little tool.
Maybe consider adding process for reporting possible vulnerabilities privately. This would usually be outlined in a file named SECURITY.md.

[nref]

Hi Jonas,

Thanks for the report. This would be the first security vulnerability I've received, so I never though to set up a reporting mechanism. I'll add that.

I will think about this potential issue. My immediate objection is, how much can BrowseRouter really do to protect the user who wants to click on such malicious links, or runs software that wants to open such links.

If the solution is as easy as escaping quotes, and we don't already do that, I'm happy to do it.

Thanks for not submitting an AI-generated report. It's nice to hear from a real person!

BTW as it happens I speak German. Grüße aus Tennessee.

[Fijo]

Hi Doug,

thanks for the quick reply and also I've been to Nashville last year. It's a nice city ;-)

Anyway, I thought I'd add this.
Basically the thing that got me thinking in the first place, is how you handle the process arguments as one big string rather than an array of strings.
I just thought this may change the behaviour, of how something is passed into the real browser binary, if it's going through this tool.

With best regards,
Jonas