Title: [112347] trunk/Source/WebCore
- Revision
- 112347
- Author
- [email protected]
- Date
- 2012-03-27 17:58:20 -0700 (Tue, 27 Mar 2012)
Log Message
Reinforce Geolocation to prevent accidental leak of the user position
https://bugs.webkit.org/show_bug.cgi?id=82396
Patch by Benjamin Poulain <[email protected]> on 2012-03-27
Reviewed by Adam Barth.
It is very important not to provide the position of the user to a page
unless the user authorize it.
The code used to make it easy to cause such problems, because any part
of the Geolocation object could invoke the success callback directly.
This patch add encapsulation for all the attributes of GeoNotifier,
and add extra guards for the two callbacks.
In the case of the success callback, we do one extra check before sending
the value to the bindings.
* Modules/geolocation/Geolocation.cpp:
(WebCore::Geolocation::GeoNotifier::runSuccessCallback):
(WebCore::Geolocation::GeoNotifier::runErrorCallback):
(WebCore):
(WebCore::Geolocation::GeoNotifier::stopTimer):
(WebCore::Geolocation::GeoNotifier::timerFired):
(WebCore::Geolocation::startRequest):
(WebCore::Geolocation::sendError):
(WebCore::Geolocation::sendPosition):
(WebCore::Geolocation::stopTimer):
(WebCore::Geolocation::extractNotifiersWithCachedPosition):
(WebCore::Geolocation::startUpdating):
* Modules/geolocation/Geolocation.h:
(WebCore::Geolocation::isAllowed):
(Geolocation):
(GeoNotifier):
(WebCore::Geolocation::GeoNotifier::options):
(WebCore::Geolocation::GeoNotifier::useCachedPosition):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (112346 => 112347)
--- trunk/Source/WebCore/ChangeLog 2012-03-28 00:52:40 UTC (rev 112346)
+++ trunk/Source/WebCore/ChangeLog 2012-03-28 00:58:20 UTC (rev 112347)
@@ -1,3 +1,41 @@
+2012-03-27 Benjamin Poulain <[email protected]>
+
+ Reinforce Geolocation to prevent accidental leak of the user position
+ https://bugs.webkit.org/show_bug.cgi?id=82396
+
+ Reviewed by Adam Barth.
+
+ It is very important not to provide the position of the user to a page
+ unless the user authorize it.
+
+ The code used to make it easy to cause such problems, because any part
+ of the Geolocation object could invoke the success callback directly.
+
+ This patch add encapsulation for all the attributes of GeoNotifier,
+ and add extra guards for the two callbacks.
+
+ In the case of the success callback, we do one extra check before sending
+ the value to the bindings.
+
+ * Modules/geolocation/Geolocation.cpp:
+ (WebCore::Geolocation::GeoNotifier::runSuccessCallback):
+ (WebCore::Geolocation::GeoNotifier::runErrorCallback):
+ (WebCore):
+ (WebCore::Geolocation::GeoNotifier::stopTimer):
+ (WebCore::Geolocation::GeoNotifier::timerFired):
+ (WebCore::Geolocation::startRequest):
+ (WebCore::Geolocation::sendError):
+ (WebCore::Geolocation::sendPosition):
+ (WebCore::Geolocation::stopTimer):
+ (WebCore::Geolocation::extractNotifiersWithCachedPosition):
+ (WebCore::Geolocation::startUpdating):
+ * Modules/geolocation/Geolocation.h:
+ (WebCore::Geolocation::isAllowed):
+ (Geolocation):
+ (GeoNotifier):
+ (WebCore::Geolocation::GeoNotifier::options):
+ (WebCore::Geolocation::GeoNotifier::useCachedPosition):
+
2012-03-27 Kausalya Madhusudhanan <[email protected]>
[Coverity] Address some uninitialized constructor values.
Modified: trunk/Source/WebCore/Modules/geolocation/Geolocation.cpp (112346 => 112347)
--- trunk/Source/WebCore/Modules/geolocation/Geolocation.cpp 2012-03-28 00:52:40 UTC (rev 112346)
+++ trunk/Source/WebCore/Modules/geolocation/Geolocation.cpp 2012-03-28 00:58:20 UTC (rev 112347)
@@ -120,15 +120,33 @@
void Geolocation::GeoNotifier::runSuccessCallback(Geoposition* position)
{
+ // 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;
+
m_successCallback->handleEvent(position);
}
+void Geolocation::GeoNotifier::runErrorCallback(PositionError* error)
+{
+ if (m_errorCallback)
+ m_errorCallback->handleEvent(error);
+}
+
void Geolocation::GeoNotifier::startTimerIfNeeded()
{
if (m_options->hasTimeout())
m_timer.startOneShot(m_options->timeout() / 1000.0);
}
+void Geolocation::GeoNotifier::stopTimer()
+{
+ m_timer.stop();
+}
+
void Geolocation::GeoNotifier::timerFired(Timer<GeoNotifier>*)
{
m_timer.stop();
@@ -140,8 +158,7 @@
// Test for fatal error first. This is required for the case where the Frame is
// disconnected and requests are cancelled.
if (m_fatalError) {
- if (m_errorCallback)
- m_errorCallback->handleEvent(m_fatalError.get());
+ runErrorCallback(m_fatalError.get());
// This will cause this notifier to be deleted.
m_geolocation->fatalErrorOccurred(this);
return;
@@ -314,7 +331,7 @@
// the permission state can not change again in the lifetime of this page.
if (isDenied())
notifier->setFatalError(PositionError::create(PositionError::PERMISSION_DENIED, permissionDeniedErrorMessage));
- else if (haveSuitableCachedPosition(notifier->m_options.get()))
+ else if (haveSuitableCachedPosition(notifier->options()))
notifier->setUseCachedPosition();
else if (notifier->hasZeroTimeout())
notifier->startTimerIfNeeded();
@@ -469,29 +486,22 @@
for (GeoNotifierVector::const_iterator it = notifiers.begin(); it != end; ++it) {
RefPtr<GeoNotifier> notifier = *it;
- if (notifier->m_errorCallback)
- notifier->m_errorCallback->handleEvent(error);
+ notifier->runErrorCallback(error);
}
}
void Geolocation::sendPosition(GeoNotifierVector& notifiers, Geoposition* position)
{
GeoNotifierVector::const_iterator end = notifiers.end();
- for (GeoNotifierVector::const_iterator it = notifiers.begin(); it != end; ++it) {
- RefPtr<GeoNotifier> notifier = *it;
- ASSERT(notifier->m_successCallback);
-
- notifier->m_successCallback->handleEvent(position);
- }
+ for (GeoNotifierVector::const_iterator it = notifiers.begin(); it != end; ++it)
+ (*it)->runSuccessCallback(position);
}
void Geolocation::stopTimer(GeoNotifierVector& notifiers)
{
GeoNotifierVector::const_iterator end = notifiers.end();
- for (GeoNotifierVector::const_iterator it = notifiers.begin(); it != end; ++it) {
- RefPtr<GeoNotifier> notifier = *it;
- notifier->m_timer.stop();
- }
+ for (GeoNotifierVector::const_iterator it = notifiers.begin(); it != end; ++it)
+ (*it)->stopTimer();
}
void Geolocation::stopTimersForOneShots()
@@ -538,7 +548,7 @@
GeoNotifierVector::iterator end = notifiers.end();
for (GeoNotifierVector::const_iterator it = notifiers.begin(); it != end; ++it) {
GeoNotifier* notifier = it->get();
- if (notifier->m_useCachedPosition) {
+ if (notifier->useCachedPosition()) {
if (cached)
cached->append(notifier);
} else
@@ -666,7 +676,7 @@
if (!page)
return false;
- page->geolocationController()->addObserver(this, notifier->m_options->enableHighAccuracy());
+ page->geolocationController()->addObserver(this, notifier->options()->enableHighAccuracy());
return true;
}
Modified: trunk/Source/WebCore/Modules/geolocation/Geolocation.h (112346 => 112347)
--- trunk/Source/WebCore/Modules/geolocation/Geolocation.h 2012-03-28 00:52:40 UTC (rev 112346)
+++ trunk/Source/WebCore/Modules/geolocation/Geolocation.h 2012-03-28 00:58:20 UTC (rev 112347)
@@ -61,6 +61,7 @@
void clearWatch(int watchId);
void setIsAllowed(bool);
+ bool isAllowed() const { return m_allowGeolocation == Yes; }
void positionChanged();
void setError(GeolocationError*);
@@ -68,7 +69,6 @@
private:
Geoposition* lastPosition();
- bool isAllowed() const { return m_allowGeolocation == Yes; }
bool isDenied() const { return m_allowGeolocation == No; }
explicit Geolocation(ScriptExecutionContext*);
@@ -78,14 +78,24 @@
class GeoNotifier : public RefCounted<GeoNotifier> {
public:
static PassRefPtr<GeoNotifier> create(Geolocation* geolocation, PassRefPtr<PositionCallback> positionCallback, PassRefPtr<PositionErrorCallback> positionErrorCallback, PassRefPtr<PositionOptions> options) { return adoptRef(new GeoNotifier(geolocation, positionCallback, positionErrorCallback, options)); }
-
+
+ PositionOptions* options() const { return m_options.get(); };
void setFatalError(PassRefPtr<PositionError>);
- bool hasZeroTimeout() const;
+
+ bool useCachedPosition() const { return m_useCachedPosition; }
void setUseCachedPosition();
+
void runSuccessCallback(Geoposition*);
+ void runErrorCallback(PositionError*);
+
void startTimerIfNeeded();
+ void stopTimer();
void timerFired(Timer<GeoNotifier>*);
-
+ bool hasZeroTimeout() const;
+
+ private:
+ GeoNotifier(Geolocation*, PassRefPtr<PositionCallback>, PassRefPtr<PositionErrorCallback>, PassRefPtr<PositionOptions>);
+
RefPtr<Geolocation> m_geolocation;
RefPtr<PositionCallback> m_successCallback;
RefPtr<PositionErrorCallback> m_errorCallback;
@@ -93,9 +103,6 @@
Timer<GeoNotifier> m_timer;
RefPtr<PositionError> m_fatalError;
bool m_useCachedPosition;
-
- private:
- GeoNotifier(Geolocation*, PassRefPtr<PositionCallback>, PassRefPtr<PositionErrorCallback>, PassRefPtr<PositionOptions>);
};
typedef Vector<RefPtr<GeoNotifier> > GeoNotifierVector;
_______________________________________________
webkit-changes mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo.cgi/webkit-changes
