If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

SecReview: Implement event specific signup page

VERIFIED FIXED

Status

mozilla.org
Security Assurance: Review Request
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: curtisk, Assigned: mfuller)

Tracking

Details

(Whiteboard: [completed secreview][start 2012/07/18][end 2012/07/19])

Attachments

(1 attachment)

SecReview tracking bug
Actions regarding the review of the dependent bug should be tracked here.

Comment 1

5 years ago
Ping for an update. Looks like there was some discussion in the pull request but it is moving forward.
Please work directly with yvan for updates and what not to move this bug forward. We all get a lot of bug mail so getting the proper attention may require a direct discussion with the bug owner.
Assignee: yboily → mfuller
(Assignee)

Comment 3

5 years ago
:bensternthal - I'll be working on the security review of this project. If you could help me out by sending me:
1) A URL of where this is hosted (stage, production, or otherwise) so that I can see the final implementation.
2) A brief description of exactly how this new page will interact with the reps.mozilla.org site (did you decide on adding a URL parameter as discussed in bug 765783)?

Thanks, I'll get started as soon as I have a URL to work with.
mfuller

Comment 4

5 years ago
Giorgos -> please provide matt with the above information.
Hi Matt,

1) This currently lives on 

https://github.com/mozilla/bedrock/pull/183

It hasn't been pushed to dev, stage or production yet.


2) This new page reads /callbackurl/ GET parameter and on form submit, it also POSTs to /callbackurl/. 

 * We *do not* post form data to /callbackurl/. We just "ping" callbackurl.
 * We do not expect Response data from reps.m.o to the POST request.
 * We *only* accept callbackurls that match a domain from the /trusted_domains/ list defined here:

https://github.com/mozilla/bedrock/pull/183/files#L5R14


Thanks!
(Assignee)

Comment 6

5 years ago
Hi Giorgos,

Thanks for the information. If I'm reading the code correctly, it looks like the domain validation is done via JavaScript. The issue here would be that anyone could easily modify the running JS on the page to allow any domains. However, I don't see a real issue with that since there is no data being submitted, just an empty POST. Is that something you've taken into account?

Also, just to make sure I have the flow of this down correctly, can you verify these steps are correct:
- A rep opens the signup page with his specific callback URL
- Every time that form is submitted, it submits an empty POST to that URL
- The callback URL keeps track of the number of times it was "pinged"
- Form reloads with same callback URL for cycle to repeat

Do you have the location of the reps code for what a typical callback page would look like? I don't see any issues on the sending side, I just want to make sure the receiving page is handling it correctly.

Thanks,
Matt
(In reply to Matt Fuller :mfuller from comment #6)
> Hi Giorgos,

Hi Matt

> 
> Thanks for the information. If I'm reading the code correctly, it looks like
> the domain validation is done via JavaScript. The issue here would be that
> anyone could easily modify the running JS on the page to allow any domains.

True, but if someone modifies the running JS can actually POST anywhere or do whatever with the data, no matter what we code. ;)

JS checking will prevent others from just linking to /contribute with bogus callbackurl arguments.


> However, I don't see a real issue with that since there is no data being
> submitted, just an empty POST. Is that something you've taken into account?

Yes, that's the intended behavior.

> 
> Also, just to make sure I have the flow of this down correctly, can you
> verify these steps are correct:
> - A rep opens the signup page with his specific callback URL
> - Every time that form is submitted, it submits an empty POST to that URL
> - The callback URL keeps track of the number of times it was "pinged"
> - Form reloads with same callback URL for cycle to repeat

Exactly.

> 
> Do you have the location of the reps code for what a typical callback page
> would look like? I don't see any issues on the sending side, I just want to
> make sure the receiving page is handling it correctly.

Here is the code:
https://github.com/mozilla/remo/blob/master/remo/events/views.py#L92


Thanks for your time,
Giorgos
(Assignee)

Comment 8

5 years ago
(In reply to Giorgos Logiotatidis [:giorgos] from comment #7)
> (In reply to Matt Fuller :mfuller from comment #6)

Hi Giorgos,

> True, but if someone modifies the running JS can actually POST anywhere or
> do whatever with the data, no matter what we code. ;)

You're right, just wanted to make sure nothing "important" was being validated with JS, but like you said this won't cause a problem since they can't link to it.

> Here is the code:
> https://github.com/mozilla/remo/blob/master/remo/events/views.py#L92

Looks good - I don't see any issues besides someone being able to manually ping the page from elsewhere and artificially inflating the numbers. There's no security concern there though. Additionally, you could load someone else's rep link and alter their numbers. Again, no security issue though.

Everything else looks good to me. I'll be wrapping up with this shortly after I make some final checks.

Thanks for the info!
Matt
Super, thanks! ;)
(Assignee)

Comment 10

5 years ago
I've finished up the review of this - everything looks good from a security perspective. I'm attaching my full report, but here's a quick summary:

- CSRF protection is not in place, however, the form requires a captcha which mitigates any issues there.
- Anyone can send a POST to the reps site, which could invalidate the results. However, that's not a security concern as no data is exposed. The only issue is artificial count inflation.

Thanks, I'll mark this resolved. Let me know if you have any questions!
Matt
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Whiteboard: [pending secreview][start mm/dd/yyyy][target mm/dd/yyyy] → [completed secreview][start 2012/07/18][end 2012/07/19]
(Assignee)

Comment 11

5 years ago
Created attachment 643957 [details]
Security Review Report
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.