375898 - [meta] Places security review

Status: RESOLVED WORKSFORME

(Firefox :: Bookmarks & History, defect)

Description

[(not reading, please use [e-mail] instead)]

after investigating moz-anno urls (which are not accessible by content, kudos to the original places team), Myk writes:  "we should do a Places security review at some point as a team to help root out and verify any other potential vulnerabilities."

firefox 3 will include a lot of code we've never shipped in a final release before (mozilla/toolkit/components/places and mozilla/browser/components/places), so we should do formal review.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Window, how do you want to do this?

Comment 2

[(not reading, please use [e-mail] instead)]

I had looked at the SQL statements we passed to execute() to make sure we always used bind parameters, to prevent SQL injection attacks.  I think we are pretty good, but we should audit the code more carefully.

we do have code where we build up the statement with AppendInt() instead of using bind parameters, though.

Comment 3

[Dietrich Ayala (:dietrich)]

The onsite next week would be a good time to get this started. Window/dveditz, would either of you be up for meeting then? If we don't have any process in place for formal internal sec. reviews, maybe we could write it as we go, so it could be re-used.

Comment 4

[Mike Connor [:mconnor]]

dolske is supposed to have an updated template for the design/security review stuff...

Comment 5

[Justin Dolske [:Dolske]]

For the password manager review, I used the template Schrep recently updated:

http://wiki.mozilla.org/Firefox3/Feature_Plan_Template

Comment 6

[Mike Connor [:mconnor]]

Dietrich, can you take point on getting this slated for sometime next week/the week after?  I don't think this blocks M8, but needs to be complete and have issues addressed by M9

Comment 7

[Dietrich Ayala (:dietrich)]

discussed w/ mconnor, moving to M10.

Comment 8

[Dietrich Ayala (:dietrich)]

The security review occurred on Tuesday, November 28th at 12pm PST. In attendance: mconnor, window, dveditz, jesse ruderman, ryan flint, johnath, seth spitzer, dietrich, justin dolske. (am i missing anyone?)

The notes are available here:

http://wiki.mozilla.org/Places:SecurityReview#Notes

I'll file all issues brought up as bugs, making them dependent on this bug, and will retarget this bug for M11 in order to track the follow-up bugs.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #8)
> The security review occurred on Tuesday, November 28th at 12pm PST. In
> attendance: mconnor, window, dveditz, jesse ruderman, ryan flint, johnath, seth
> spitzer, dietrich, justin dolske. (am i missing anyone?)

me!

Comment 10

[Al Billings [:abillings - ex-MoCo]]

timr and I attended...

Comment 11

[Mike Connor [:mconnor]]

the issues that are considered blocking are already on the blocker list, and there's nothing here that's sensitive, so, opening, and culling from the list.

Comment 12

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv

Comment 13

[Marco Bonardo [:mak]]

security review for firefox 3, outdated now. There is only one item left, but we pay a lot of attention in proper queries binding so that doesn't worry me.