[rekonq] Re: Review Request: SSL dialogs improvements

Thu Jul 21 13:43:14 CEST 2011

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101925/#review4928
-----------------------------------------------------------


src/sslinfodialog.cpp
<http://git.reviewboard.kde.org/r/101925/#comment4279>

    Errors may contain < or & characters. You need something like:
    
    s = s.replace(QL1S("&"), QL1S("&amp;"));
    s = s.replace(QL1S("<"), QL1S("&lt;"));


src/sslinfodialog.cpp
<http://git.reviewboard.kde.org/r/101925/#comment4280>

    All of these fields can contain & or < and need quoting as above.


src/sslinfodialog.cpp
<http://git.reviewboard.kde.org/r/101925/#comment4281>

    Using a SHA1 hash is better than MD5 since colliding certificates for MD5 have been created in practice.


src/sslinfodialog.cpp
<http://git.reviewboard.kde.org/r/101925/#comment4282>

    The isValid() function is the wrong one to use. That merely checks the certificate start and end dates, and that it has not been blacklisted (as covered in the docs). You should always use the information from the error chain, not the information from the certificate. This is true for many reasons (eg. this code would say any valid certificate is valid for ANY SITE AT ALL).
    

src/sslinfodialog.cpp
<http://git.reviewboard.kde.org/r/101925/#comment4283>

    The common name is untrusted. You need to strip out dangerous characters here (eg. a common name of ../../../.. is totall possible).
    

src/sslinfodialog.cpp
<http://git.reviewboard.kde.org/r/101925/#comment4284>

    Why does this function even exist? The errors are presented by QSslSocket as a list in the first place. Trying to parse them out of a string is asking for security problems.
    

src/urlbar/sslwidget.cpp
<http://git.reviewboard.kde.org/r/101925/#comment4285>

    This code is wrong. If the cert is null it doesn't mean the site provided a null certificate - there's no such thing. It means that no certificate was presented at all (eg. the server only supports anonymous diffie hellman encryption).


src/urlbar/sslwidget.cpp
<http://git.reviewboard.kde.org/r/101925/#comment4286>

    This needs to be quoted as above.
    

src/urlbar/sslwidget.cpp
<http://git.reviewboard.kde.org/r/101925/#comment4287>

    At least tell the user what the problem is!


src/urlbar/sslwidget.cpp
<http://git.reviewboard.kde.org/r/101925/#comment4288>

    If the certificate is not valid, the security is not low, it's non-existent since a MITM attack may be occurring. Low should be used when the server only supports weak ciphers.
    

src/urlbar/sslwidget.cpp
<http://git.reviewboard.kde.org/r/101925/#comment4289>

    This is wrong, the connection may be encrypted using anonymous diffie hellman. Most likely however something would be seriously wrong for this to occur and you should steer the user away.
    

src/urlbar/sslwidget.cpp
<http://git.reviewboard.kde.org/r/101925/#comment4290>

    Is  m_info.supportedChiperBits() a typo?


src/urlbar/sslwidget.cpp
<http://git.reviewboard.kde.org/r/101925/#comment4291>

    What does the version of the certificate have to do with the version of SSL in use? Answer - Nothing!
    
    To find out the version of SSL in use you need to ask the QSslSocket.
    

src/urlbar/sslwidget.cpp
<http://git.reviewboard.kde.org/r/101925/#comment4292>

    SSL2 is not medium security. It should be avoided at all costs.


src/urlbar/sslwidget.cpp
<http://git.reviewboard.kde.org/r/101925/#comment4293>

    If this is unknown then you have a serious problem, not medium security.


src/urlbar/sslwidget.cpp
<http://git.reviewboard.kde.org/r/101925/#comment4294>

    This message does not reflect what the code above does.
    

- Richard J.


On July 11, 2011, 10:21 p.m., Andrea Diamantini wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101925/
> -----------------------------------------------------------
> 
> (Updated July 11, 2011, 10:21 p.m.)
> 
> 
> Review request for rekonq.
> 
> 
> Summary
> -------
> 
> The patch, available on the remote repo in the branch called "SSL_Dialogs_Improvements",  provides 2 new GUIs, similar to the ones provided by Google Chrome, to notify users about SSL infos.
> The patch allows also to export the certificate in binary form. Further upgrades in the SSL management have to be done in kdelibs.
> 
> "Side" change to render the same informations Chrom* does is about history: upgraded HISTORY_VERSION and added a firstDateTimeVisit entry in the HistoryTime struct.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt f623adf 
>   src/history/historymanager.h 8650bd1 
>   src/history/historymanager.cpp 8113add 
>   src/history/historymodels.h 8cb1a5d 
>   src/history/historymodels.cpp 2cab8ef 
>   src/sslinfo.ui PRE-CREATION 
>   src/sslinfodialog.h PRE-CREATION 
>   src/sslinfodialog.cpp PRE-CREATION 
>   src/sslinfodialog_p.h 72f1679 
>   src/urlbar/sslwidget.h PRE-CREATION 
>   src/urlbar/sslwidget.cpp PRE-CREATION 
>   src/urlbar/urlbar.cpp 078dc6a 
>   src/webpage.h 09977bf 
>   src/webpage.cpp 9499d6f 
> 
> Diff: http://git.reviewboard.kde.org/r/101925/diff
> 
> 
> Testing
> -------
> 
> Tested against the following sites:
> - https://localhost, with an untrusted and expired certificate
> - https://encrypted.google.com
> - https://paypal.com
> - https://sites.nea.org/JoinNea/
> 
> In all these sites, rekonq behaves similar to what Chrom* does.
> 
> 
> Screenshots
> -----------
> 
> SSL Widget
>   http://git.reviewboard.kde.org/r/101925/s/196/
> SSL info Dialog
>   http://git.reviewboard.kde.org/r/101925/s/197/
> 
> 
> Thanks,
> 
> Andrea
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/rekonq/attachments/20110721/e2d15fb0/attachment-0001.htm