Bug 82396 - Reinforce Geolocation to prevent accidental leak of the user position
Summary: Reinforce Geolocation to prevent accidental leak of the user position
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-27 16:42 PDT by Benjamin Poulain
Modified: 2012-03-27 17:58 PDT (History)
2 users (show)

See Also:


Attachments
Patch (8.80 KB, patch)
2012-03-27 16:58 PDT, Benjamin Poulain
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2012-03-27 16:42:31 PDT
Some simple changes can be done to enhance Geolocation in case of programming mistake.
Comment 1 Benjamin Poulain 2012-03-27 16:42:58 PDT
Related to <rdar://problem/11106417>
Comment 2 Benjamin Poulain 2012-03-27 16:58:20 PDT
Created attachment 134171 [details]
Patch
Comment 3 Adam Barth 2012-03-27 17:03:57 PDT
Comment on attachment 134171 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134171&action=review

> Source/WebCore/Modules/geolocation/Geolocation.cpp:128
> +    // If we are here and the Geolocation permission is not approved, something has
> +    // gone horribly wrong.
> +    // We bail out to avoid any privacy issue.
> +    ASSERT(m_geolocation->isAllowed());
> +    if (!m_geolocation->isAllowed())
> +        return;

Should we CRASH() rather than return here?
Comment 4 Benjamin Poulain 2012-03-27 17:10:08 PDT
> > Source/WebCore/Modules/geolocation/Geolocation.cpp:128
> > +    // If we are here and the Geolocation permission is not approved, something has
> > +    // gone horribly wrong.
> > +    // We bail out to avoid any privacy issue.
> > +    ASSERT(m_geolocation->isAllowed());
> > +    if (!m_geolocation->isAllowed())
> > +        return;
> 
> Should we CRASH() rather than return here?

I have considered that but that seemed a bit extreme.

Do you have any input on what is bad enough that we should take down the whole browser? It is difficult to know it an error comes from a security breach or a programming mistake.
Comment 5 Adam Barth 2012-03-27 17:45:03 PDT
> Do you have any input on what is bad enough that we should take down the whole browser? It is difficult to know it an error comes from a security breach or a programming mistake.

It's up to you.  The nice thing about crashing in impossible situations is that we'll get crash reports and we can fix the issue.  :)
Comment 6 Benjamin Poulain 2012-03-27 17:58:54 PDT
Committed r112347: <http://trac.webkit.org/changeset/112347>