unshiftio / url-parse

Small footprint URL parser that works seamlessly across Node.js and browser environments.
http://unshift.io
MIT License
1.03k stars 104 forks source link

Are XSS scenarios considered to be valid? #228

Closed Scara31 closed 2 years ago

Scara31 commented 2 years ago

Good day, I actually tried to contact you regarding this via security@3rd-Eden.com, but didn't get a reply :( From a few reports it seemed to me that you may consider an XSS as valid issue. E.g.: https://hackerone.com/reports/496293 Here the researcher uses javascript:, though the report is a bit obscure. And here - https://huntr.dev/bounties/57124ed5-4b68-4934-8325-2c546257f2e4/ - the researcher adds this

const parse = require('./index.js')
const express = require('express')
const app = express()
const port = 3000

url = parse("\bjavascript:alert(1)")

console.log(url)

app.get('/', (req, res) => {
  if (url.protocol !== "javascript:") {res.send("<a href=\'" + url.href + "\'>CLICK ME!</a>")}
})

app.listen(port, () => {
  console.log(`Example app listening on port ${port}`)
})

as an additional PoC.

So I wanted to kindly ask you: do you believe that these scenarios are valid and in case url-parse fails to separate javascript: as a protocol - it is an issue? Thanks a lot in advance!

lpinca commented 2 years ago

This is an issue with many URL parsers so I'm inclined to say no. Here are some examples:

$ node
Welcome to Node.js v17.5.0.
Type ".help" for more information.
> url.parse('javascript&colon;prompt(1)')
Url {
  protocol: null,
  slashes: null,
  auth: null,
  host: null,
  port: null,
  hostname: null,
  hash: null,
  search: null,
  query: null,
  pathname: 'javascript&colon;prompt(1)',
  path: 'javascript&colon;prompt(1)',
  href: 'javascript&colon;prompt(1)'
}
>
$ python3
Python 3.8.9 (default, Oct 26 2021, 07:25:54) 
[Clang 13.0.0 (clang-1300.0.29.30)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from urllib.parse import urlparse
>>> urlparse("javascript&colon;prompt(1)")
ParseResult(scheme='', netloc='', path='javascript&colon', params='prompt(1)', query='', fragment='')
>>> from urllib.parse import urlunparse
>>> urlunparse(urlparse("javascript&colon;prompt(1)"))
'javascript&colon;prompt(1)'
>>>
$ docker run -it --rm php:cli-alpine php --version
PHP 8.1.2 (cli) (built: Jan 21 2022 21:38:22) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.1.2, Copyright (c) Zend Technologies
$ docker run -it --rm php:cli-alpine php -a       
Interactive shell

php > var_dump(parse_url("javascript&colon;prompt(1)"));
array(1) {
  ["path"]=>
  string(26) "javascript&colon;prompt(1)"
}
php >
$ cat test.go 
package main

import (
    "fmt"
    "net/url"
)

func main() {
    s := "javascript&colon;prompt(1)"

    u, err := url.Parse(s)
    if err != nil {
        panic(err)
    }

    fmt.Printf("%+v\n", *u)
    fmt.Println(u)
}
$ docker run -it --rm -v $(pwd):/tmp golang:alpine go run /tmp/test.go
{Scheme: Opaque: User: Host: Path:javascript&colon;prompt(1) RawPath:javascript&colon;prompt(1) ForceQuery:false RawQuery: Fragment: RawFragment:}
javascript&colon;prompt(1)
$ ruby -v
ruby 2.6.8p205 (2021-07-07 revision 67951) [universal.x86_64-darwin21]
$ cat test.rb 
require 'uri'

uri = URI("javascript&colon;prompt(1)")

printf("scheme: \"%s\"\n", uri.scheme)
printf("host: \"%s\"\n", uri.host)
printf("path: \"%s\"\n", uri.path)
printf("query: \"%s\"\n", uri.query)
printf("fragment: \"%s\"\n", uri.fragment)
printf("href: \"%s\"\n", uri.to_s)
$ ruby test.rb 
scheme: ""
host: ""
path: "javascript&colon;prompt(1)"
query: ""
fragment: ""
href: "javascript&colon;prompt(1)"

We already received a report about this on huntr.

Haxatron commented 2 years ago

I would say, yes they should when it's clearly the parser's fault (ref: see recently reported vulnerability in https://bugs.python.org/issue43882). The & in & colon; scenario is NOT the parser's fault because & should already be HTML-encoded to & amp; by a proper HTML sanitization library.

lpinca commented 2 years ago

If I understand correctly that python issue talks about stripping off '\n' and '\t'. url-parse already does that.

Haxatron commented 2 years ago

yep, was just pointing out that xss impact should be considered valid if its a fault with the url-parser

lpinca commented 2 years ago

The & in & colon; scenario is NOT the parser's fault because & should already be HTML-encoded to & amp; by a proper HTML sanitization library.

I'm not sure I understand. If the parser receives &amp; instead of &, that would break the parsing. Do you mean sanitizing the result before adding it to the href attribute of the Anchor element?

Scara31 commented 2 years ago

Do you mean sanitizing the result before adding it to the href attribute of the Anchor element?

Yes, I was wondering whether url-parse considers XSS sanitization of HTML-encoded entities or not. Thank you very much for your detailed answer and also thanks to @Haxatron for joining the thread.

Haxatron commented 2 years ago

Yes, & colon; scenario is not for the parser to handle because the & should be sanitized to & amp; by any HTML sanitizer.

On Thu, 24 Feb 2022, 03:10 Luigi Pinca, @.***> wrote:

The & in & colon; scenario is NOT the parser's fault because & should already be HTML-encoded to & amp; by a proper HTML sanitization library.

I'm not sure I understand. If the parser receives & instead of & that would break parsing. Do you mean sanitizing the result before adding it to the href attribute of the Anchor element?

— Reply to this email directly, view it on GitHub https://github.com/unshiftio/url-parse/issues/228#issuecomment-1049120830, or unsubscribe https://github.com/notifications/unsubscribe-auth/ASHOYPKOKIMENLK3UNX7TCTU4UWLTANCNFSM5PEKXD6Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you commented.Message ID: @.***>