microcosm-cc / bluemonday

bluemonday: a fast golang HTML sanitizer (inspired by the OWASP Java HTML Sanitizer) to scrub user generated content of XSS
https://github.com/microcosm-cc/bluemonday
BSD 3-Clause "New" or "Revised" License
3.14k stars 176 forks source link

AllowElements iframe doesn't work #66

Closed jtamary closed 4 years ago

jtamary commented 6 years ago

Hey

I've tried to add iframe to the whitelist, but it still sanitize iframes.

Example code:

package main

import (
    "github.com/microcosm-cc/bluemonday"
    "fmt"
)

func main() {
    raw := "<iframe></iframe>"
    p := bluemonday.NewPolicy()
    p.AllowElements("iframe")
     res := p.Sanitize(raw)
     if res != raw {
        fmt.Printf("got: %s\n", res)
     } else {
        fmt.Println("happy, happy, joy, joy!")
    }
}

Any help will be awesome.

Thanks, Jonathan

jtamary commented 6 years ago

@buro9 any thoughts?

buro9 commented 6 years ago

As per comment in the PR... yeah, that would be the thing to change. But I am curious as to what use an iframe is that has no attributes?

mfridman commented 5 years ago

@buro9 What's the rationale for preventing something like the following, this is an example of an iframe copy/pasted directly from youtube:

in := `<iframe width="560" height="315" src="https://www.youtube.com/embed/BIvezCVcsYs" frameborder="0" allow="accelerometer; autoplay; encrypted-media; gyroscope; picture-in-picture" allowfullscreen></iframe>`

p := bluemonday.UGCPolicy()
p.AllowElements("iframe")

out := p.Sanitize(in)

fmt.Printf("out: %q\n", out)
// out: ""
buro9 commented 5 years ago

@mfridman it is a whitelist:

    p.AllowElements("iframe")
    p.AllowAttrs("width").Matching(bluemonday.Number).OnElements("iframe")
    p.AllowAttrs("height").Matching(bluemonday.Number).OnElements("iframe")
    p.AllowAttrs("src").OnElements("iframe")
    p.AllowAttrs("frameborder").Matching(bluemonday.Number).OnElements("iframe")
    p.AllowAttrs("allow").Matching(regexp.MustCompile(`[a-z; -]*`)).OnElements("iframe")
    p.AllowAttrs("allowfullscreen").OnElements("iframe")
mfridman commented 5 years ago

@buro9 Ah thanks, it's whitelist all the way down. Which, upon reflection, is the entire point. Not sure why I thought it'd be possible to "blanket" an entire element.

Tricky part with iframes is different providers might have different attributes, but that's okay. The attributes grow organically over time.

Thanks!

buro9 commented 5 years ago

I do not trust IFRAMEs and do not trust 3rd party content by allowing them through my bluemonday policies.

Instead I have a list of trusted third parties https://github.com/microcosm-cc/microcosm/blob/master/models/link_embedmedia.go#L229-L240 and when I see links of a particular format that matches those (i.e. a YouTube video) then I will replace the link with a version of the embed HTML that I control https://github.com/microcosm-cc/microcosm/blob/master/models/link_embedmedia.go#L303-L344 . This happens as the final step in my content processing, and because I control the embed HTML I trust that, and it does require me to also trust the third parties.

This means that on the forums I run a person can just paste in the YouTube URI and it gets auto-embedded without risking letting any 3rd party content through, for example https://www.lfgss.com/conversations/136127/?offset=2525#comment14682852 and I keep the original URI above the embed so that if the third party fails then the link will always work.

The other advantage of this approach is that it means that you could update the content-security-policy to match that list of third parties to de-risk your site further (from untrusted third parties).

mfridman commented 5 years ago

@buro9 That's really useful information, thanks for sharing that!

buro9 commented 4 years ago

Closing as answered