search

mozilla / gameon

INACTIVE - http://mzl.la/ghe-archive - Simple playdoh site to support the gameon 2012 challenge
BSD 3-Clause "New" or "Revised" License
0 stars 6 forks source link

Atul to continuously security review master branch #24

Closed toolness closed 11 years ago

toolness commented 11 years ago

Hey, I'm just starting this issue to keep track of how much I've code reviewed so far for bug 813182... is that ok, or do you want me to keep this information somewhere else?

toolness commented 11 years ago

So far I've reviewed up to (but not including) e9f7a688eb11ad71bc6aa14e2536ddfce39b0159.

toolness commented 11 years ago

Now reviewed up to (and including) cd32733fb76159c61d13c06478ff08d003c29e72. Looks good so far!

rossbruniges commented 11 years ago

Nah - this works for me. If you could also keep the bugzilla updated that would also be cool, as I'm guessing that IT might be watching that for action...

toolness commented 11 years ago

Ok, good idea... I will update bug 813182 regularly.

toolness commented 11 years ago

Ok, now reviewed up to (and including) 893ef86a56cda4bbce416c998ea1d1f6074efeeb. Filed #50 and #51 as a result of my reviews so far, aside from the other inline comments I've made.

I also need to know if I need to review micawber too, or if it's already been security reviewed, since it seems to be generating HTML from user input that we're marking as safe.

rossbruniges commented 11 years ago

So I would say we certainly haven't security reviewed micawber - but hopefully with a mixture of your tests and some further explanations in the code we can either work around or come to a decision on it's usage

davidascher commented 11 years ago

I would ask on webdev list for alternative tools to do what you need.

toolness commented 11 years ago

Oh, wait! I forgot to mention another thing... micawber appears to be using the oembed standard, which requires a network request to convert every youtube/vimeo URL into its corresponding embed HTML. This is why my test suite in #50 runs so slowly. Could this become a performance issue?

Also, section 2.3.4.2 of the oembed standard mentions "Consumers may wish to load the HTML in an off-domain iframe to avoid XSS vulnerabilities". It looks like the vimeo and youtube providers in micawber's bootstrap_basic() work over http, which technically means that DNS spoofing on the server-side could cause things to go awry--but then again, I don't know if server-side DNS spoofing should be considered much of a threat.

One solution to resolve all of these issues may just be to write our own, fairly straightforward regexp code to do this for us with youtube and vimeo embeds. The youtube link-to-embed algorithm is nicely documented and seems straightforward enough, at least. Couldn't find an official source on the vimeo version but we could infer it.

rossbruniges commented 11 years ago

I'm thinking we might end up having to knock this on the head - I think the performance issue will be relatively small and when you're making something in weeks it's a hit we'll have to take.

The whole idea of using an external lib was to keep things simple and quick, and no one has asked for the functionality it requires; it's just something nice I thought that we could do.

toolness commented 11 years ago

I like the functionality, so I implemented a (hopefully) secure implementation and added it to #50. The implementation is likely less flexible than micawber, but it's better than nothing, and it doesn't have any glaring security issues that I can think of.

toolness commented 11 years ago

I like the functionality, so I implemented a (hopefully) secure implementation and added it to #50. The implementation is likely less flexible than micawber, but it's better than nothing, and it doesn't have any glaring security issues that I can think of.

rossbruniges commented 11 years ago

Atul - can you update the bugzilla with the info. Looking into things all the security issues you've noticed have now been fixed.

The IT guys are waiting on your for starting on the deployment work so would be good to get them ready!

Cheers,

Ross

toolness commented 11 years ago

Will do - I was just looking through the Secure Coding Guidelines though and realized I wasn't focusing on some of the guidelines mentioned in the Uploads section. Will take another pass through to make sure everything is OK there, and then update Bugzilla with the info.

rossbruniges commented 11 years ago

So I've done all that Mark asked on Ignite so it should be good - publishing off a separate domain I don't think is going to be feasible for this release. Thankfully using an imageField clears up a bunch of the other stuff :)

All the rest was marked as low priority stuff on Ignite so hopefully not a blocker!

toolness commented 11 years ago

Oh, nice! Cool, all is well then. I just posted bug 813182 comment 10 giving this my stamp of approval, for what that's worth. Let me know if you need anything else!

rossbruniges commented 11 years ago

Atul - this is cool to close now yeah?

toolness commented 11 years ago

Yep, I think so!

rossbruniges commented 11 years ago

Whoop whoop!