Re: The netsec thread

[Lars Ingebrigtsen]

OK, I'm on the balcony and it's code review time.  Let's see...  I think
this is the correct diff range.

Overall, I think it looks really good.  Whether each check is on the
right level or not I think is something we'll find out by just using it,
and we can adjust them individually if they seem to get in our way.

Comments and questions about the code:

> +** New function 'network-lookup-address-info'.
> +This does IPv4 and/or IPv6 address lookups on hostnames.

I'm assuming this will be removed again since we got the new getaddrinfo
function?

>    "List of CA bundle location filenames or a function returning said list.
> +If a file path contains glob wildcards, they will be expanded.

Hm.  This seems like a good idea, but do we do this in other similar
variables?  And would perhaps regexp syntax make more sense than glob
syntax?

> -;;;###autoload
> -(defcustom gnutls-min-prime-bits 256
> -  ;; Several mail servers send fewer bits than the GnuTLS default.
> -  ;; Currently, 256 appears to be a reasonable choice (Bug#11267).
> +(defcustom gnutls-min-prime-bits nil

As I've said before, I don't think this makes much sense.  But in any
case, the variable is obsolescent (since GnuTLS has said so), so perhaps
we should just mark it obsolete and tell people to use the priority
string to control these things.

> +(defcustom gnutls-crlfiles
> +  '(
> +    "/etc/grid-security/certificates/*.crl.pem"
> +    )
> +  "List of CRL file paths or a function returning said list.
> +If a file path contains glob wildcards, they will be expanded.

And same thing here about glob vs regexp.

>  The following values are possible:
 
> -`low': Absolutely no checks are performed.
> -`medium': This is the default level, should be reasonable for most usage.
> -`high': This warns about additional things that many people would
> -not find useful.
> -`paranoid': On this level, the user is queried for most new connections.
> +`low': Check for problems known before Edward Snowden.
> +`medium': Default.  Suitable for most circumstances.
> +`high': Warns about additional issues not enabled in `medium' due to
> +compatibility concerns.

I don't think it makes much sense to talk about Snowden as if that's
something people are meant to understand.  And, as I've said before,
`paranoid' should stay.

> +(defcustom nsm-trust-local-network nil
> +  "Disable warnings when visiting trusted hosts on local networks.

This doc string doesn't seem to define what a "local" network is.  Is
that localhost or machines in your current netmask, or something more
wider?

And while I do see that it may be useful (under any of these
definitions of "local"), the NSM already provides per-host settings,
so I think there's potential for confusion here.

> +(defcustom nsm-tls-checks
> +  '(;; Pre-Snowden Known Weaknesses
> +    (nsm-tls-check-version                . low)
> +    (nsm-tls-check-compression            . low)
> +    (nsm-tls-check-renegotiation-info-ext . low)

(etc)

Calling protocol checks "TLS" checks isn't future proof.  We've
already had one politically motivated name change (from SSL to TLS)
and we may have another.  And besides, many of these checks are also
valid for SSL, so it's just confusing.

Call them `nsm-protocol-check' and stick the -- back in.  And having
the entire function name instead of just the bit after "check--" makes
it more tedious for people to remove/add their own functions.

> +(defun nsm-should-check (host)
> +  "Determines whether NSM should check for TLS problems for HOST.
> +
> +If `nsm-trust-local-network' is or returns non-nil, and if the
> +host address is a localhost address, a machine address, a direct
> +link or a private network address, this function returns
> +nil.  Non-nil otherwise."

What do you mean by "machine address"?  The MAC address?  If you mean
IP address, it's perfectly valid to have TLS on a non-named IP
address.  1.0.0.1 does that for DNS over HTTPS last I heard, and
that's definitely a service you should verify, well, everything on.

> +         (and (or (and (functionp nsm-trust-local-network)
> +                       (funcall nsm-trust-local-network))
> +                  nsm-trust-local-network)
> +              (if ipv4?
> +                  (or
> +                   ;; (10.x.x.x) private
> +                   (eq (aref address 0) 10)
> +                   ;; (172.16.x.x) private
> +                   (and (eq (aref address 0) 172)
> +                        (eq (aref address 0) 16))
> +                   ;; (192.168.x.x) private

Oh, by "local network", you mean one using a private IP range?  Makes
sense.

> +(defun nsm-tls-check-rsa-kx (host port status &optional settings)

In Emacs functions we try to avoid abbreviations unless they're very
common.  kx is too obscure; say key-exchange instead.

> +Reference:
> +
> +Sheffer, Holz, Saint-Andre (May 2015).  \"Recommendations for Secure
> +Use of Transport Layer Security (TLS) and Datagram Transport Layer
> +Security (DTLS)\", \"(4.1.  General Guidelines)\"
> +`https://tools.ietf.org/html/rfc7525\#section-4.1'"

[...]

> +GnuTLS authors (2018). \"GnuTLS Manual 4.3.3 Anonymous
> +authentication\",
> +`https://www.gnutls.org/manual/gnutls.html\#Anonymous-authentication'"

Heh heh.  I like all the references, but perhaps it's a lot of URLs to
keep updated?  Perhaps not?

> +          (let* ((accept-choices '((?a "always" "Accept this certificate 
> this session and for all future sessions.")
> +                                   (?s "session only" "Accept this 
> certificate this session only.")
> +                                   (?n "no" "Refuse to use this certificate, 
> and close the connection.")
> +                                   (?d "details" "See certificate details")))
> +                 (details-choices '((?b "backward page" "See previous page")
> +                                    (?f "forward page" "See next page")
> +                                    (?n "next" "Next certificate")
> +                                    (?p "previous" "Previous certificate")
> +                                    (?q "quit" "Quit details view")))

See previous messages about the UI.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no