Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#4245 closed defect (fixed)

Browse: show a error if ssl certificate is wrong

Reported by: godiard Owned by: humitos
Priority: Normal Milestone:
Component: Browse Version: Unspecified
Severity: Unspecified Keywords: r+, olpc-test-passed
Cc: humitos, greenfeld, dsd Distribution/OS: Unspecified
Bug Status: Unconfirmed

Description

See more info in techteam

Attachments (3)

0001-Use-system-certificate-authorities-for-SSL-certifica.patch (1.3 KB) - added by humitos 8 years ago.
Sam's patch that does not allow any invalid certificate
0001-Display-security-as-a-lock-broken-lock-icon-for-secu.patch (8.6 KB) - added by manuq 8 years ago.
Unfinished patch
0001-Display-security-as-a-lock-icon-for-secured-pages-SL.patch (12.9 KB) - added by manuq 8 years ago.
Candidate patch.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 8 years ago by humitos

  • Owner changed from manuq to humitos
  • Status changed from new to accepted

comment:2 follow-up: Changed 8 years ago by dsd

  • Cc dsd added

After some discussion it seems like webkit/webkitgtk does not present the API to allow us to do a really complete job here (pop up warning messages, allow users to confirm certificate exceptions, apply these considerations to embedded content as well as the main page, and whatever else).

The two immediate options we have here are:

  1. A one-line patch from Sam to disable loading of any site that doesn't provide valid SSL certificates (please post here) (note: activation.laptop.org doesn't currently have a valid cert)
  2. Show a broken lock icon like epiphany does but keep loading anyway.

If I missed any options that don't involve writing loads of code and modifying webkit, etc, please post them here.

I vote for the 2nd option. We have often applied a "if in doubt, do what epiphany does" mindset to Browse, and I think this is again valid here.

Changed 8 years ago by humitos

Sam's patch that does not allow any invalid certificate

comment:3 in reply to: ↑ 2 Changed 8 years ago by humitos

I vote for the 2nd option. We have often applied a "if in doubt, do what epiphany does" mindset to Browse, and I think this is again valid here.

Well, Epiphany shows the broken lock but when you click on it Epiphany shows a dialogue with all the information of the certificate. I understand that we don't want to show that dialogue at this time on the cycle.

comment:4 follow-up: Changed 8 years ago by dsd

Is that how epiphany behaves on the XO? Or did you test on some other setup?

comment:5 in reply to: ↑ 4 Changed 8 years ago by humitos

Replying to dsd:

Is that how epiphany behaves on the XO? Or did you test on some other setup?

Oh, sorry. I forgot to mention that. It's on the XO-4 13.1.0 build 14.

comment:6 Changed 8 years ago by greenfeld

  • Cc greenfeld added; Cerlyn removed

Changed 8 years ago by manuq

Unfinished patch

comment:7 Changed 8 years ago by manuq

The attached patch tries to do the second option, in the same way Epiphany does here. However from Python soup_message_get_https_status is always returning False for the first output, which means is not using https. I have left the logging calls to show it. My test URLs were:

comment:8 Changed 8 years ago by manuq

Oh and the change I did in the Soup session is based on this:

http://git.gnome.org/browse/epiphany/tree/embed/ephy-embed-single.c#n457

comment:9 Changed 8 years ago by manuq

  • Keywords r? added

comment:10 Changed 8 years ago by manuq

Sorry the patch is working on os15, please test.

comment:11 Changed 8 years ago by manuq

Or maybe wait until I update it to work when switching tabs.

comment:12 Changed 8 years ago by humitos

  • Keywords r- added; r? removed

Well, the patch looks good and works like it says. I checked the sites that are mentioned on this ticket and some others as well, like Facebook, PayPal and some SSL cert sellers. It shows a broken lock when the SSL is invalid for the site visited and show a lock when the SSL is valid.

There is just a little issue in the log file when a PDF tab is opened:

Traceback (most recent call last):
  File "/home/olpc/Activities/Browse.activity/webtoolbar.py", line 352, in __switch_page_cb
    self._connect_to_browser(tabbed_view.props.current_browser)
  File "/home/olpc/Activities/Browse.activity/webtoolbar.py", line 369, in _connect_to_browser
    self._set_security_status(self._browser.security_status)
AttributeError: 'DummyBrowser' object has no attribute 'security_status'
/home/olpc/Activities/Browse.activity/webtoolbar.py:356: Warning: gsignal.c:2576: instance `0x12fa730' has no handler with id `15343'
  self._browser.disconnect(self._uri_changed_hid)

I know that we discussed about this yesterday and we want a simple solution similar how Epiphany works but I'd like to add two comments on this patch to consider them in the future:

  1. ssl-use-system-ca-file: setting this property to True we are trusting on the ca-files that we have on our system. In some way this could be dangerous if the user has an invalid CA file on his system.
  2. widget.get_main_frame(): we are checking the certificate just for the main frame. So, if the main frame has a valid certificate we are saying that you are in a trusted site when that is not totally true because the "real website" could be inside an iframe or doing something weird via javascript.

I'd say, fix the traceback issue, push this patch and go back later in the future with the best solution considering all the possibilities.

Changed 8 years ago by manuq

Candidate patch.

comment:13 Changed 8 years ago by manuq

  • Keywords r? added; r- removed

Thanks for the review, updated PDF DummyBrowser and updated the patch.

comment:14 Changed 8 years ago by humitos

  • Keywords r+ added; r? removed

Good work manuq! It's everything ok. Go ahead pushing it! :D

comment:15 Changed 8 years ago by manuq

  • Keywords olpc-test-pending added
  • Resolution set to fixed
  • Status changed from accepted to closed

Thanks, pushed dcb5f998 .

comment:16 Changed 8 years ago by greenfeld

  • Keywords olpc-test-passed added; olpc-test-pending removed

We show a lock icon in Browse-148 indicating the status of the currently shown main frame, but are no better than Epiphany and at least a few other WebKitGTK browsers in a bunch of not-so-edge cases. Bug reports about these various issues are already filed elsewhere, and have been for a while.

We are waiting to hear back to see if there is consensus about how to universally fix this across all the various WebKitGTK browsers.

comment:17 Changed 8 years ago by dnarvaez

  • Milestone 0.98 deleted

Milestone 0.98 deleted

Note: See TracTickets for help on using tickets.