The netsec thread

The netsec thread

[Lars Ingebrigtsen]

[-- Attachment #1: Type: text/plain, Size: 237 bytes --]

I still haven't looked at the code for the netsec branch, but I've got
that scheduled for Sunday.  *crosses fingers*

But that doesn't mean that we can't bikeshed some UI stuff in the
meantime.  :-)

Here's the current NSM info buffer:


[-- Attachment #2: old.png --]
[-- Type: image/png, Size: 31628 bytes --]

[-- Attachment #3: Type: text/plain, Size: 30 bytes --]


And here's Jimmy's new one:


[-- Attachment #4: new.png --]
[-- Type: image/png, Size: 38682 bytes --]

[-- Attachment #5: Type: text/plain, Size: 1040 bytes --]


I like the underlining, indentation and *bullet points, but it's looking
like it's moving more in the direction of a TLS debugging buffer than
something that a user has much chance of understanding or navigating.
(The old one also has that problem, but to a lesser degree.)

For instance, displaying the full x.50x/RFC4514 string is a turn-off and
looks like line noise to most people, I think.  Breaking out the three
bits that are of interest, the CN from the issuer, the O from the
recipient, and the host name, is more readable.  (Not to mention that
the strings are usually too long and will wrap on common
configurations.)

And in the opposite direction, breaking out all the encryption stuff
into their own lines doesn't make much sense, I think.  The session
details could be reserved for the `d'etails buffer.  "Encrypt-then-MAC"
sounds so...  internal.  :-)

The explanation line ("... is insecure ...") hasn't been folded
correctly, and it says "reasons" even if there's just one reason.

The addition of the details is great:


[-- Attachment #6: new-details.png --]
[-- Type: image/png, Size: 61710 bytes --]

[-- Attachment #7: Type: text/plain, Size: 329 bytes --]


The keystrokes should be more standard, though: SPC for forward and DEL
for back (perhaps in addition to the one you've got).  And I was unsure
what "quit" would do -- quit the entire connection or just the details
buffer?

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

Re: The netsec thread

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Fri, 20 Jul 2018 13:33:03 +0200
> Cc: Jimmy Yuen Ho Wong <wyuenho@gmail.com>
> 
> I like the underlining, indentation and *bullet points, but it's looking
> like it's moving more in the direction of a TLS debugging buffer than
> something that a user has much chance of understanding or navigating.
> (The old one also has that problem, but to a lesser degree.)
> 
> For instance, displaying the full x.50x/RFC4514 string is a turn-off and
> looks like line noise to most people, I think.  Breaking out the three
> bits that are of interest, the CN from the issuer, the O from the
> recipient, and the host name, is more readable.  (Not to mention that
> the strings are usually too long and will wrap on common
> configurations.)

Would it make sense to have a button "More details" there, which would
then display these details?

Re: The netsec thread

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Would it make sense to have a button "More details" there, which would
> then display these details?

That would be nice, but when the buffer is displayed, Emacs is waiting
for multiple choice prompt, so the user can't really click anything.

Hm...  perhaps that could be fixed by avoiding the multiple choice
prompt, and making that buffer into its own major mode with keys bound
to what are now choice items?

That is, instead of saying "Continue connecting? ([a]lways, [n]o)" etc
via that mechanism, it would just `message' that line and bind the `a'
(etc) key to the appropriate action?

It would require quite a bit of rewriting to go from "modal" interaction
to "mode based" interaction, though.

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

Re: The netsec thread

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> It would require quite a bit of rewriting to go from "modal" interaction
> to "mode based" interaction, though.

Since the nsm is basically interrupting the control flow of
`open-network-connection', we'd...  er...  have to enter a recursive
editing session so that we can resume the program control afterwards.

Err...  I think it sounds kinda complicated.  :-)

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

Re: The netsec thread

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org,  wyuenho@gmail.com
> Date: Fri, 20 Jul 2018 14:33:22 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Would it make sense to have a button "More details" there, which would
> > then display these details?
> 
> That would be nice, but when the buffer is displayed, Emacs is waiting
> for multiple choice prompt, so the user can't really click anything.
> 
> Hm...  perhaps that could be fixed by avoiding the multiple choice
> prompt, and making that buffer into its own major mode with keys bound
> to what are now choice items?

We do something similar in the prompt that select-safe-coding-system
presents, so "we have the technology".

Re: The netsec thread

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> We do something similar in the prompt that select-safe-coding-system
> presents, so "we have the technology".

Ah, great!

Then I think that would be nice -- I'm not much fan of modal prompts for
stuff like this, because that makes it difficult for the user to
interact with the buffer presented.  For instance, if we use a non-modal
interface, then the scrolling problems in the new "details" disappear:
The user can just scroll normally.

And, as you suggest, we could have a button to expand some details "in
buffer".

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

Re: The netsec thread

[Jimmy Yuen Ho Wong]

Woooo I like this bikeshedding :) Thanks in advance for checking out
the netsec branch. Just a heads up tho, I plan to push my OCSP stuff
this weekend, let me know if you prefer me doing it before or after
your review.

Abobut the issued to and issued by, there are a couple of reasons I
just dumped out the the whole DN, mainly because of DV, OV and EV
certs put different and sometime irrelevent things in the RDNs. But
obvious, current iteration isn't ideal either. Perhaps I should sort
the RDNs, and always outpunt CN=...O=...OU=.... in that order, and
properly wrap them.

As to session info, I can probably merge the TLS extensions into a single line.


On Fri, Jul 20, 2018 at 12:33 PM, Lars Ingebrigtsen <larsi@gnus.org> wrote:
> I still haven't looked at the code for the netsec branch, but I've got
> that scheduled for Sunday.  *crosses fingers*
>
> But that doesn't mean that we can't bikeshed some UI stuff in the
> meantime.  :-)
>
> Here's the current NSM info buffer:
>
>
>
> And here's Jimmy's new one:
>
>
>
> I like the underlining, indentation and *bullet points, but it's looking
> like it's moving more in the direction of a TLS debugging buffer than
> something that a user has much chance of understanding or navigating.
> (The old one also has that problem, but to a lesser degree.)
>
> For instance, displaying the full x.50x/RFC4514 string is a turn-off and
> looks like line noise to most people, I think.  Breaking out the three
> bits that are of interest, the CN from the issuer, the O from the
> recipient, and the host name, is more readable.  (Not to mention that
> the strings are usually too long and will wrap on common
> configurations.)
>
> And in the opposite direction, breaking out all the encryption stuff
> into their own lines doesn't make much sense, I think.  The session
> details could be reserved for the `d'etails buffer.  "Encrypt-then-MAC"
> sounds so...  internal.  :-)
>
> The explanation line ("... is insecure ...") hasn't been folded
> correctly, and it says "reasons" even if there's just one reason.
>
> The addition of the details is great:
>
>
>
> The keystrokes should be more standard, though: SPC for forward and DEL
> for back (perhaps in addition to the one you've got).  And I was unsure
> what "quit" would do -- quit the entire connection or just the details
> buffer?
>
> --
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no
>
>

Re: The netsec thread

[Jimmy Yuen Ho Wong]

OMG my typing skills...

On Fri, Jul 20, 2018 at 1:55 PM, Jimmy Yuen Ho Wong <wyuenho@gmail.com> wrote:
> Woooo I like this bikeshedding :) Thanks in advance for checking out
> the netsec branch. Just a heads up tho, I plan to push my OCSP stuff
> this weekend, let me know if you prefer me doing it before or after
> your review.
>
> Abobut the issued to and issued by, there are a couple of reasons I
> just dumped out the the whole DN, mainly because of DV, OV and EV
> certs put different and sometime irrelevent things in the RDNs. But
> obvious, current iteration isn't ideal either. Perhaps I should sort
> the RDNs, and always outpunt CN=...O=...OU=.... in that order, and
> properly wrap them.
>
> As to session info, I can probably merge the TLS extensions into a single line.
>
>
> On Fri, Jul 20, 2018 at 12:33 PM, Lars Ingebrigtsen <larsi@gnus.org> wrote:
>> I still haven't looked at the code for the netsec branch, but I've got
>> that scheduled for Sunday.  *crosses fingers*
>>
>> But that doesn't mean that we can't bikeshed some UI stuff in the
>> meantime.  :-)
>>
>> Here's the current NSM info buffer:
>>
>>
>>
>> And here's Jimmy's new one:
>>
>>
>>
>> I like the underlining, indentation and *bullet points, but it's looking
>> like it's moving more in the direction of a TLS debugging buffer than
>> something that a user has much chance of understanding or navigating.
>> (The old one also has that problem, but to a lesser degree.)
>>
>> For instance, displaying the full x.50x/RFC4514 string is a turn-off and
>> looks like line noise to most people, I think.  Breaking out the three
>> bits that are of interest, the CN from the issuer, the O from the
>> recipient, and the host name, is more readable.  (Not to mention that
>> the strings are usually too long and will wrap on common
>> configurations.)
>>
>> And in the opposite direction, breaking out all the encryption stuff
>> into their own lines doesn't make much sense, I think.  The session
>> details could be reserved for the `d'etails buffer.  "Encrypt-then-MAC"
>> sounds so...  internal.  :-)
>>
>> The explanation line ("... is insecure ...") hasn't been folded
>> correctly, and it says "reasons" even if there's just one reason.
>>
>> The addition of the details is great:
>>
>>
>>
>> The keystrokes should be more standard, though: SPC for forward and DEL
>> for back (perhaps in addition to the one you've got).  And I was unsure
>> what "quit" would do -- quit the entire connection or just the details
>> buffer?
>>
>> --
>> (domestic pets only, the antidote for overdose, milk.)
>>    bloggy blog: http://lars.ingebrigtsen.no
>>
>>

Re: The netsec thread

[Lars Ingebrigtsen]

Jimmy Yuen Ho Wong <wyuenho@gmail.com> writes:

> Woooo I like this bikeshedding :) Thanks in advance for checking out
> the netsec branch. Just a heads up tho, I plan to push my OCSP stuff
> this weekend, let me know if you prefer me doing it before or after
> your review.

Just go ahead with your work; there's no need to synchronise with me.
:-)

> Abobut the issued to and issued by, there are a couple of reasons I
> just dumped out the the whole DN, mainly because of DV, OV and EV
> certs put different and sometime irrelevent things in the RDNs. But
> obvious, current iteration isn't ideal either. Perhaps I should sort
> the RDNs, and always outpunt CN=...O=...OU=.... in that order, and
> properly wrap them.

Well, I think that most of those data points are of virtually no
interest to the end user, so they shouldn't be displayed at all (in the
non-detailed view).  I.e., I think the way the current NSM displays
those data points is fine.  (But then I would, wouldn't I? :-))

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

Re: The netsec thread

[Jimmy Yuen Ho Wong]

>
> I.e., I think the way the current NSM displays
> those data points is fine.  (But then I would, wouldn't I? :-))
>

Indeed you would :)

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

Re: The netsec thread

[Jimmy Yuen Ho Wong]

>> +** 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?
>

Yep.

>>    "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?
>

Nah. This has already been looked at weeks ago, no need to
over-engineer it further as I believe most people will think of glob
when dealing with file paths. Besides, Emacs's regex isn't
particularly nice unless Dan elects to retrofit PCRE into Emacs :P

The reason I need glob is IGTF's fetch-crl will put ~100s CRL PEM
files into the file system, it's very cumbersome to specify them
1-by-1.

>> -;;;###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.
>

Sorry I got lost in that giant thread.
`gnutls_dh_set_prime_bits` is only deprecated on GnuTLS 3.1.7+. Are we
dropping support for all version < 3.1.7? I'd be super happy to do it
if that's the case and remove this var and the C code entirely.

>> -`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.

I'd be happy to, but I have no idea what 'low means anymore. In fact,
if you think about impact as a 2-dimensional variable as opposed to
this linear scale, you'd have a quadrant defined by "impact" and
"compatibility". Hi-impact-hi-compat should be enabled without
question, which roughly corresponds to 'low. Hi-impact-lo-compat and
Lo-impact-hi-compat do not exist as a concept in NSM ATM.
Lo-impact-lo-compat is 'paranoid? But what would be in it and why
bother?

> And, as I've said before,
> `paranoid' should stay.
>

Eli's use case has already been taken cared of by
`nsm-trust-local-network`. `paranoid has been aliased to `high for
backward compatibility.

Robert do you still object to removing the `paranoid level? I've
removed that prompt that askes for permission on every TLS connection
due to crying-wolf effect.

If there isn't an objection from people who've found use for it, I'd
really like to try without 'paranoid on master later before declaring
it insufficient.


> 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.
>

The TLS working group wasn't even willing to call TLS 1.3[1] TLS 2.0
even when it's a major departure from it. I doubt we need to worry
about extra work to change a name. YAGNI applies.

[1]: https://www.ietf.org/mail-archive/web/tls/current/msg20938.html

> 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.
>

`nsm-tls-checks` is already a defcustom. It's super easy to add and
remove a function. You can defun whatever name you want and add to it,
and click [-] to remove. Using name mangling magic to fish out a check
function makes defcustom super-awkward, and AFAIK, no other emacs core
setting does it this way.

>> +(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.
>

I mean 0.0.0.0/8. I'm not sure what the proper name is or if I even
need to deal with it. What do you think?

>
>> +(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.
>

Will do.

>> +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?
>

These provide justification for the checks and make it easy for
everyone to fact-check my rationale. Most of them are just RFC URLs,
they never fail to resolve.

>> +          (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.
>

Will update that later.


Thanks for reviewing!

Re: The netsec thread

[Jimmy Yuen Ho Wong]

On Fri, Jul 20, 2018 at 1:45 PM, Lars Ingebrigtsen <larsi@gnus.org> wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
>
>> We do something similar in the prompt that select-safe-coding-system
>> presents, so "we have the technology".
>
> Ah, great!
>
> Then I think that would be nice -- I'm not much fan of modal prompts for
> stuff like this, because that makes it difficult for the user to
> interact with the buffer presented.  For instance, if we use a non-modal
> interface, then the scrolling problems in the new "details" disappear:
> The user can just scroll normally.
>
> And, as you suggest, we could have a button to expand some details "in
> buffer".
>

That sound great, but what is this select-safe-coding-system and how
do I look at its UI?

Re: The netsec thread

[Brett Gilio]

Jimmy Yuen Ho Wong writes:

> On Fri, Jul 20, 2018 at 1:45 PM, Lars Ingebrigtsen 
> <larsi@gnus.org> wrote:
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>>> We do something similar in the prompt that 
>>> select-safe-coding-system
>>> presents, so "we have the technology".
>>
>> Ah, great!
>>
>> Then I think that would be nice -- I'm not much fan of modal 
>> prompts for
>> stuff like this, because that makes it difficult for the user 
>> to
>> interact with the buffer presented.  For instance, if we use a 
>> non-modal
>> interface, then the scrolling problems in the new "details" 
>> disappear:
>> The user can just scroll normally.
>>
>> And, as you suggest, we could have a button to expand some 
>> details "in
>> buffer".
>>
>
> That sound great, but what is this select-safe-coding-system and 
> how
> do I look at its UI?

Agreed.

Eli, could you point me to the instance of 
select-safe-coding-system? I
am not able to find it in documentation or help.

-- 
Brett M. Gilio
Free Software Foundation, Member
https://parabola.nu | https://emacs.org

Re: The netsec thread

[Eli Zaretskii]

> From: Jimmy Yuen Ho Wong <wyuenho@gmail.com>
> Date: Mon, 23 Jul 2018 02:52:23 +0100
> Cc: Eli Zaretskii <eliz@gnu.org>, Emacs-Devel devel <emacs-devel@gnu.org>
> 
> That sound great, but what is this select-safe-coding-system and how
> do I look at its UI?

lisp/international/mule-cmds.el, look at
select-safe-coding-system-interactively, a subroutine of
select-safe-coding-system.  In a nutshell, it prepares and pops up a
buffer with special content, then prompts the user to select one of
several alternatives.

Re: The netsec thread

[Robert Pluim]

Jimmy Yuen Ho Wong <wyuenho@gmail.com> writes:

>> And, as I've said before,
>> `paranoid' should stay.
>>
>
> Eli's use case has already been taken cared of by
> `nsm-trust-local-network`. `paranoid has been aliased to `high for
> backward compatibility.
>
> Robert do you still object to removing the `paranoid level? I've
> removed that prompt that askes for permission on every TLS connection
> due to crying-wolf effect.

As Iʼve said before: I donʼt think many people need to be prompted
every time a TLS connection is set up from emacs to a host thatʼs
never been seen before, but I do, as I need to inspect the connection
parameters. Yes itʼs annoying, but I can live with self-imposed
annoyance.

> If there isn't an objection from people who've found use for it, I'd
> really like to try without 'paranoid on master later before declaring
> it insufficient.

I guess I could always add my own function into 'high, but Iʼd prefer
it if it was available by default.

>>> +(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.
>>

Iʼm a bit dubious about the whole 'nsm-trust-local-network' stuff. nsm
already stores per-host settings, so why the need to check for RFC
1918 addresses? Besides, I can easily set things up so that Iʼm using
RFC1918 addresses, but the packets would definitely not be going over
what you'd think of as a 'local' or 'private' network.

Iʼd thought this was going to be something like 'trust this subnet',
which I can see people wanting, although Iʼd recommend against them
using. That would require that nsm-should-check pass the host to the
user-specified nsm-trust-local-network function (or you define a
'nsm-trusted-by-default-networks' variable).

> I mean 0.0.0.0/8. I'm not sure what the proper name is or if I even
> need to deal with it. What do you think?

If you mean localhost, then you have:

             (or
              ;; (0.x.x.x) this machine
              (eq (aref address 0) 0)
              ;; (127.x.x.x) localhost
              (eq (aref address 0) 0))

where that second eq should check against 127, I think. I can think of
no situation in which 0.0.0.0/8 would be something you need to check
against (nor can I see how you'd ever see such a value).

Robert

Re: The netsec thread

[Lars Ingebrigtsen]

Jimmy Yuen Ho Wong <wyuenho@gmail.com> writes:

>> 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?
>
> Nah. This has already been looked at weeks ago, no need to
> over-engineer it further as I believe most people will think of glob
> when dealing with file paths. Besides, Emacs's regex isn't
> particularly nice unless Dan elects to retrofit PCRE into Emacs :P

.* isn't particularly difficult to write.  The question is whether Emacs
has a history of using regexps or globs in these kinds of variables -- I
don't know, but we should find out and use whatever is the Emacs way.

> The reason I need glob is IGTF's fetch-crl will put ~100s CRL PEM
> files into the file system, it's very cumbersome to specify them
> 1-by-1.

What's IGTF?

> Sorry I got lost in that giant thread.
> `gnutls_dh_set_prime_bits` is only deprecated on GnuTLS 3.1.7+. Are we
> dropping support for all version < 3.1.7? I'd be super happy to do it
> if that's the case and remove this var and the C code entirely.

Sure, but making it obsolete won't mean that we drop anything until,
like, 2023, and by then GnuTLS will be up to 5.4.13.

>> I don't think it makes much sense to talk about Snowden as if that's
>> something people are meant to understand.
>
> I'd be happy to, but I have no idea what 'low means anymore.

But mentioning "Snowden" doesn't really help, either.  I think most
people understands that "low security" means, like, not very secure, and
that's sufficient.

> Robert do you still object to removing the `paranoid level? I've
> removed that prompt that askes for permission on every TLS connection
> due to crying-wolf effect.
>
> If there isn't an objection from people who've found use for it, I'd
> really like to try without 'paranoid on master later before declaring
> it insufficient.

Robert objects, apparently.  And in any case, we can't just remove
settings like this: We make them obsolete for a few years if we want
them gone, and then we remove them.

>> 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.
>
> The TLS working group wasn't even willing to call TLS 1.3[1] TLS 2.0
> even when it's a major departure from it. I doubt we need to worry
> about extra work to change a name. YAGNI applies.

There is no extra work, because we shouldn't call the functions
something containing "tls".

> `nsm-tls-checks` is already a defcustom. It's super easy to add and
> remove a function. You can defun whatever name you want and add to it,
> and click [-] to remove. Using name mangling magic to fish out a check
> function makes defcustom super-awkward, and AFAIK, no other emacs core
> setting does it this way.

There's a bunch of "feature" setting that do not include full function
names.

>>> +(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.
>
> I mean 0.0.0.0/8. I'm not sure what the proper name is or if I even
> need to deal with it. What do you think?

If I understand correctly, 0.0.0.0/8 is only used when you don't really
have an IP address yet?  It doesn't sound like something that's a very
common use case, but if you cover that case, you'll have to call it
something other than "machine address".  :-)

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

Re: The netsec thread

[Andreas Schwab]

On Jul 23 2018, Jimmy Yuen Ho Wong <wyuenho@gmail.com> wrote:

> I mean 0.0.0.0/8. I'm not sure what the proper name is or if I even
> need to deal with it. What do you think?

0/8 is only valid as a source address, and only during initialisation.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: The netsec thread

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Jimmy Yuen Ho Wong <wyuenho@gmail.com>
>> Date: Mon, 23 Jul 2018 02:52:23 +0100
>> Cc: Eli Zaretskii <eliz@gnu.org>, Emacs-Devel devel <emacs-devel@gnu.org>
>> 
>> That sound great, but what is this select-safe-coding-system and how
>> do I look at its UI?
>
> lisp/international/mule-cmds.el, look at
> select-safe-coding-system-interactively, a subroutine of
> select-safe-coding-system.  In a nutshell, it prepares and pops up a
> buffer with special content, then prompts the user to select one of
> several alternatives.

Hm...  I ran:

(select-safe-coding-system-interactively 1 3 (find-coding-systems-region 1 3) nil)

and it just popped up a buffer with some text and then called
`read-coding-system', which is kinda close to what the NSM prompt is
doing already.  Only the NSM prompt calls a y-or-n-p-equivalent, so that
you can't enter any other keys then the required ones, while
`read-coding-system' uses the more freeform minibuffer read...

So it's still modal, and not the more "just drop us into a buffer with a
special mode and continue execution after the user hits a keystroke",
isn't it?

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

Re: The netsec thread

[Andy Moreton]

On Mon 23 Jul 2018, Lars Ingebrigtsen wrote:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Jimmy Yuen Ho Wong <wyuenho@gmail.com>
>>> Date: Mon, 23 Jul 2018 02:52:23 +0100
>>> Cc: Eli Zaretskii <eliz@gnu.org>, Emacs-Devel devel <emacs-devel@gnu.org>
>>> 
>>> That sound great, but what is this select-safe-coding-system and how
>>> do I look at its UI?
>>
>> lisp/international/mule-cmds.el, look at
>> select-safe-coding-system-interactively, a subroutine of
>> select-safe-coding-system.  In a nutshell, it prepares and pops up a
>> buffer with special content, then prompts the user to select one of
>> several alternatives.
>
> Hm...  I ran:
>
> (select-safe-coding-system-interactively 1 3 (find-coding-systems-region 1 3) nil)
>
> and it just popped up a buffer with some text and then called
> `read-coding-system', which is kinda close to what the NSM prompt is
> doing already.  Only the NSM prompt calls a y-or-n-p-equivalent, so that
> you can't enter any other keys then the required ones

... except if you do press a different key, NSM gets stuck and does not
accept new valid keys (other than ^G to quit).

    AndyM

Re: The netsec thread

[Jimmy Yuen Ho Wong]

On Mon, Jul 23, 2018 at 2:31 PM, Andy Moreton <andrewjmoreton@gmail.com> wrote:
> On Mon 23 Jul 2018, Lars Ingebrigtsen wrote:
>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>>>> From: Jimmy Yuen Ho Wong <wyuenho@gmail.com>
>>>> Date: Mon, 23 Jul 2018 02:52:23 +0100
>>>> Cc: Eli Zaretskii <eliz@gnu.org>, Emacs-Devel devel <emacs-devel@gnu.org>
>>>>
>>>> That sound great, but what is this select-safe-coding-system and how
>>>> do I look at its UI?
>>>
>>> lisp/international/mule-cmds.el, look at
>>> select-safe-coding-system-interactively, a subroutine of
>>> select-safe-coding-system.  In a nutshell, it prepares and pops up a
>>> buffer with special content, then prompts the user to select one of
>>> several alternatives.
>>
>> Hm...  I ran:
>>
>> (select-safe-coding-system-interactively 1 3 (find-coding-systems-region 1 3) nil)
>>
>> and it just popped up a buffer with some text and then called
>> `read-coding-system', which is kinda close to what the NSM prompt is
>> doing already.  Only the NSM prompt calls a y-or-n-p-equivalent, so that
>> you can't enter any other keys then the required ones
>
> ... except if you do press a different key, NSM gets stuck and does not
> accept new valid keys (other than ^G to quit).
>
>     AndyM
>
>

That sounds like a lot of trouble just to deal with ^G

Re: The netsec thread

[Andy Moreton]

On Mon 23 Jul 2018, Jimmy Yuen Ho Wong wrote:

> On Mon, Jul 23, 2018 at 2:31 PM, Andy Moreton <andrewjmoreton@gmail.com> wrote:
>> On Mon 23 Jul 2018, Lars Ingebrigtsen wrote:
>>
>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>
>>>>> From: Jimmy Yuen Ho Wong <wyuenho@gmail.com>
>>>>> Date: Mon, 23 Jul 2018 02:52:23 +0100
>>>>> Cc: Eli Zaretskii <eliz@gnu.org>, Emacs-Devel devel <emacs-devel@gnu.org>
>>>>>
>>>>> That sound great, but what is this select-safe-coding-system and how
>>>>> do I look at its UI?
>>>>
>>>> lisp/international/mule-cmds.el, look at
>>>> select-safe-coding-system-interactively, a subroutine of
>>>> select-safe-coding-system.  In a nutshell, it prepares and pops up a
>>>> buffer with special content, then prompts the user to select one of
>>>> several alternatives.
>>>
>>> Hm...  I ran:
>>>
>>> (select-safe-coding-system-interactively 1 3 (find-coding-systems-region 1 3) nil)
>>>
>>> and it just popped up a buffer with some text and then called
>>> `read-coding-system', which is kinda close to what the NSM prompt is
>>> doing already.  Only the NSM prompt calls a y-or-n-p-equivalent, so that
>>> you can't enter any other keys then the required ones
>>
>> ... except if you do press a different key, NSM gets stuck and does not
>> accept new valid keys (other than ^G to quit).
>
> That sounds like a lot of trouble just to deal with ^G

You have not understood what I meant: the existing code is buggy. It is
supposed to allow the user to press a limited number of keys to decide
what to do with the presented certificate. however if you press any
other key, the NSM gets stuck and you have to quit with ^G to kill it.

    AndyM

Re: The netsec thread

[Jimmy Yuen Ho Wong]

On Mon, Jul 23, 2018 at 9:17 AM, Robert Pluim <rpluim@gmail.com> wrote:
> Jimmy Yuen Ho Wong <wyuenho@gmail.com> writes:
>
>>> And, as I've said before,
>>> `paranoid' should stay.
>>>
>>
>> Eli's use case has already been taken cared of by
>> `nsm-trust-local-network`. `paranoid has been aliased to `high for
>> backward compatibility.
>>
>> Robert do you still object to removing the `paranoid level? I've
>> removed that prompt that askes for permission on every TLS connection
>> due to crying-wolf effect.
>
> As Iʼve said before: I donʼt think many people need to be prompted
> every time a TLS connection is set up from emacs to a host thatʼs
> never been seen before, but I do, as I need to inspect the connection
> parameters. Yes itʼs annoying, but I can live with self-imposed
> annoyance.
>

Deep human packet inspector Robert :)

Would you mind expanding a bit more on that need to inspect the
connection params when you connect to a host you've never seen before?
Currently the prompt doesn't really show you nearly enough to inspect
connection params. I want to make sure your need is taken cared of
properly (I'm contemplating an actual debug mode that prompts after
every handshake) without introducing something into emacs that's so
far only useful for one person.

>> If there isn't an objection from people who've found use for it, I'd
>> really like to try without 'paranoid on master later before declaring
>> it insufficient.
>
> I guess I could always add my own function into 'high, but Iʼd prefer
> it if it was available by default.
>

I can alternatively resurrect that functionality as a separate check
that's available but not added to any levels. You can add it to any
level in Customize easy enough, but not so easy that a naive user
would enable in the hope for more security. Would this be an
acceptable compromise?

Re: The netsec thread

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Date: Mon, 23 Jul 2018 10:17:52 +0200
> Cc: Lars Ingebrigtsen <larsi@gnus.org>, Emacs-Devel devel <emacs-devel@gnu.org>
> Iʼm a bit dubious about the whole 'nsm-trust-local-network' stuff. nsm
> already stores per-host settings, so why the need to check for RFC
> 1918 addresses? Besides, I can easily set things up so that Iʼm using
> RFC1918 addresses, but the packets would definitely not be going over
> what you'd think of as a 'local' or 'private' network.
> 
> Iʼd thought this was going to be something like 'trust this subnet',
> which I can see people wanting, although Iʼd recommend against them
> using.

Sounds like a good idea to me, yes.

Re: The netsec thread

[Robert Pluim]

Jimmy Yuen Ho Wong <wyuenho@gmail.com> writes:

> On Mon, Jul 23, 2018 at 9:17 AM, Robert Pluim <rpluim@gmail.com> wrote:
>> Jimmy Yuen Ho Wong <wyuenho@gmail.com> writes:
>>
>>>> And, as I've said before,
>>>> `paranoid' should stay.
>>>>
>>>
>>> Eli's use case has already been taken cared of by
>>> `nsm-trust-local-network`. `paranoid has been aliased to `high for
>>> backward compatibility.
>>>
>>> Robert do you still object to removing the `paranoid level? I've
>>> removed that prompt that askes for permission on every TLS connection
>>> due to crying-wolf effect.
>>
>> As Iʼve said before: I donʼt think many people need to be prompted
>> every time a TLS connection is set up from emacs to a host thatʼs
>> never been seen before, but I do, as I need to inspect the connection
>> parameters. Yes itʼs annoying, but I can live with self-imposed
>> annoyance.
>>
>
> Deep human packet inspector Robert :)

You donʼt want to know :-)

> Would you mind expanding a bit more on that need to inspect the
> connection params when you connect to a host you've never seen before?
> Currently the prompt doesn't really show you nearly enough to inspect
> connection params. I want to make sure your need is taken cared of
> properly (I'm contemplating an actual debug mode that prompts after
> every handshake) without introducing something into emacs that's so
> far only useful for one person.

The current info is enough for me. Anything else I need I can get from
packet captures.

Iʼm sure Lars added it for a reason, so there must be at least two of
us :-)

>>> If there isn't an objection from people who've found use for it, I'd
>>> really like to try without 'paranoid on master later before declaring
>>> it insufficient.
>>
>> I guess I could always add my own function into 'high, but Iʼd prefer
>> it if it was available by default.
>>
>
> I can alternatively resurrect that functionality as a separate check
> that's available but not added to any levels. You can add it to any
> level in Customize easy enough, but not so easy that a naive user
> would enable in the hope for more security. Would this be an
> acceptable compromise?

Yes, that would be fine

Thanks

Robert

Re: The netsec thread

[Jimmy Yuen Ho Wong]

>> The reason I need glob is IGTF's fetch-crl will put ~100s CRL PEM
>> files into the file system, it's very cumbersome to specify them
>> 1-by-1.
>
> What's IGTF?
>

The only way that I know of where you can get a set of CRL in PEM
without writing a program. It's availble in every Debian distro as
igtf-policy-bundle or something like it.

https://www.igtf.net/

>> Sorry I got lost in that giant thread.
>> `gnutls_dh_set_prime_bits` is only deprecated on GnuTLS 3.1.7+. Are we
>> dropping support for all version < 3.1.7? I'd be super happy to do it
>> if that's the case and remove this var and the C code entirely.
>
> Sure, but making it obsolete won't mean that we drop anything until,
> like, 2023, and by then GnuTLS will be up to 5.4.13.
>

I take this means removing the related code entirely but not declare
in the NEWS file like 26.1 that we are now requiring GnuTLS >= 3.1.7.
Sounds good to me.

>
> But mentioning "Snowden" doesn't really help, either.  I think most
> people understands that "low security" means, like, not very secure, and
> that's sufficient.
>

Fine by me. So, go back to the original wording and just checks the
certs and fingerprints on low is what you are saying?


>>> 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.
>>
>> The TLS working group wasn't even willing to call TLS 1.3[1] TLS 2.0
>> even when it's a major departure from it. I doubt we need to worry
>> about extra work to change a name. YAGNI applies.
>
> There is no extra work, because we shouldn't call the functions
> something containing "tls".
>

But that's not what you argue for originally. I can drop the `-tls-`
bit if that makes that part of the shed more palettable.

>> `nsm-tls-checks` is already a defcustom. It's super easy to add and
>> remove a function. You can defun whatever name you want and add to it,
>> and click [-] to remove. Using name mangling magic to fish out a check
>> function makes defcustom super-awkward, and AFAIK, no other emacs core
>> setting does it this way.
>
> There's a bunch of "feature" setting that do not include full function
> names.
>

???

Re: The netsec thread

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Jimmy Yuen Ho Wong <wyuenho@gmail.com>,  emacs-devel@gnu.org
> Date: Mon, 23 Jul 2018 14:46:44 +0200
> 
> > lisp/international/mule-cmds.el, look at
> > select-safe-coding-system-interactively, a subroutine of
> > select-safe-coding-system.  In a nutshell, it prepares and pops up a
> > buffer with special content, then prompts the user to select one of
> > several alternatives.
> 
> Hm...  I ran:
> 
> (select-safe-coding-system-interactively 1 3 (find-coding-systems-region 1 3) nil)
> 
> and it just popped up a buffer with some text and then called
> `read-coding-system', which is kinda close to what the NSM prompt is
> doing already.

That's not the best demonstration of what it does.  Try evaluating the
below in *scratch*, and then play with the links it creates to point
out characters that cannot be encoded:

  (find-file "try-select-safe-coding-system")
  (set-buffer-file-coding-system 'latin-1)
  (prefer-coding-system 'latin-1)
  (insert "text with smileys 😺 😇 😎")
  (save-buffer)

I didn't mean to say you must copy what it does, just to point out an
example where we ask a question, but let the user click or type RET on
active parts of the buffer to see additional information.  I also
don't see any particular reason to avoid modal questions, as the user
can always return to the minibuffer after looking at the additional
info, as in the above demonstration.

HTH

Re: The netsec thread

[Jimmy Yuen Ho Wong]

On Mon, Jul 23, 2018 at 4:04 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Robert Pluim <rpluim@gmail.com>
>> Date: Mon, 23 Jul 2018 10:17:52 +0200
>> Cc: Lars Ingebrigtsen <larsi@gnus.org>, Emacs-Devel devel <emacs-devel@gnu.org>
>> Iʼm a bit dubious about the whole 'nsm-trust-local-network' stuff. nsm
>> already stores per-host settings, so why the need to check for RFC
>> 1918 addresses? Besides, I can easily set things up so that Iʼm using
>> RFC1918 addresses, but the packets would definitely not be going over
>> what you'd think of as a 'local' or 'private' network.
>>
>> Iʼd thought this was going to be something like 'trust this subnet',
>> which I can see people wanting, although Iʼd recommend against them
>> using.
>
> Sounds like a good idea to me, yes.
>

That's exactly what it is, and why `network-trust-local-network`
defaults to nil. I can fix the docstrings.

Re: The netsec thread

[Robert Pluim]

Jimmy Yuen Ho Wong <wyuenho@gmail.com> writes:

> On Mon, Jul 23, 2018 at 4:04 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>>> From: Robert Pluim <rpluim@gmail.com>
>>> Date: Mon, 23 Jul 2018 10:17:52 +0200
>>> Cc: Lars Ingebrigtsen <larsi@gnus.org>, Emacs-Devel devel <emacs-devel@gnu.org>
>>> Iʼm a bit dubious about the whole 'nsm-trust-local-network' stuff. nsm
>>> already stores per-host settings, so why the need to check for RFC
>>> 1918 addresses? Besides, I can easily set things up so that Iʼm using
>>> RFC1918 addresses, but the packets would definitely not be going over
>>> what you'd think of as a 'local' or 'private' network.
>>>
>>> Iʼd thought this was going to be something like 'trust this subnet',
>>> which I can see people wanting, although Iʼd recommend against them
>>> using.
>>
>> Sounds like a good idea to me, yes.
>>
>
> That's exactly what it is, and why `network-trust-local-network`
> defaults to nil. I can fix the docstrings.

That may be its intent, but thatʼs not what the implementation in
nsm.el does, as far as I can tell. All I see are comparisons to
RFC1918 addresses.

Robert

Re: The netsec thread

[Lars Ingebrigtsen]

Jimmy Yuen Ho Wong <wyuenho@gmail.com> writes:

> I want to make sure your need is taken cared of
> properly (I'm contemplating an actual debug mode that prompts after
> every handshake) without introducing something into emacs that's so
> far only useful for one person.

I feel I have to remind you that we're not introducing something; it's
you who insist on removing something without any period of obsoleting
the feature.

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

Re: The netsec thread

[Lars Ingebrigtsen]

Jimmy Yuen Ho Wong <wyuenho@gmail.com> writes:

>>> Sorry I got lost in that giant thread.
>>> `gnutls_dh_set_prime_bits` is only deprecated on GnuTLS 3.1.7+. Are we
>>> dropping support for all version < 3.1.7? I'd be super happy to do it
>>> if that's the case and remove this var and the C code entirely.
>>
>> Sure, but making it obsolete won't mean that we drop anything until,
>> like, 2023, and by then GnuTLS will be up to 5.4.13.
>
> I take this means removing the related code entirely but not declare
> in the NEWS file like 26.1 that we are now requiring GnuTLS >= 3.1.7.
> Sounds good to me.

No, marking something as obsolete means that we leave the variable and
code in Emacs for a couple of release cycles.

> Fine by me. So, go back to the original wording and just checks the
> certs and fingerprints on low is what you are saying?

Yes.

>> There is no extra work, because we shouldn't call the functions
>> something containing "tls".
>
> But that's not what you argue for originally. I can drop the `-tls-`
> bit if that makes that part of the shed more palettable.

That is what I was arguing for originally.

>>> `nsm-tls-checks` is already a defcustom. It's super easy to add and
>>> remove a function. You can defun whatever name you want and add to it,
>>> and click [-] to remove. Using name mangling magic to fish out a check
>>> function makes defcustom super-awkward, and AFAIK, no other emacs core
>>> setting does it this way.
>>
>> There's a bunch of "feature" setting that do not include full function
>> names.
>
> ???

I'm not sure what you're asking...

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

Re: The netsec thread

[Lars Ingebrigtsen]

Andy Moreton <andrewjmoreton@gmail.com> writes:

> You have not understood what I meant: the existing code is buggy. It is
> supposed to allow the user to press a limited number of keys to decide
> what to do with the presented certificate. however if you press any
> other key, the NSM gets stuck and you have to quit with ^G to kill it.

That sounds like a bug.  When I hit a different key it just says that
it's an invalid key and I can then hit the right key.

Do you have a test case?

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

Re: The netsec thread

[Jimmy Yuen Ho Wong]

On Mon, Jul 23, 2018 at 4:46 PM, Lars Ingebrigtsen <larsi@gnus.org> wrote:
> Jimmy Yuen Ho Wong <wyuenho@gmail.com> writes:
>
>>>> Sorry I got lost in that giant thread.
>>>> `gnutls_dh_set_prime_bits` is only deprecated on GnuTLS 3.1.7+. Are we
>>>> dropping support for all version < 3.1.7? I'd be super happy to do it
>>>> if that's the case and remove this var and the C code entirely.
>>>
>>> Sure, but making it obsolete won't mean that we drop anything until,
>>> like, 2023, and by then GnuTLS will be up to 5.4.13.
>>
>> I take this means removing the related code entirely but not declare
>> in the NEWS file like 26.1 that we are now requiring GnuTLS >= 3.1.7.
>> Sounds good to me.
>
> No, marking something as obsolete means that we leave the variable and
> code in Emacs for a couple of release cycles.
>

What do you want to do with the 256-bit standard value then?

Re: The netsec thread

[Noam Postavsky]

On 23 July 2018 at 11:46, Lars Ingebrigtsen <larsi@gnus.org> wrote:

>>>> `nsm-tls-checks` is already a defcustom. It's super easy to add and
>>>> remove a function. You can defun whatever name you want and add to it,
>>>> and click [-] to remove. Using name mangling magic to fish out a check
>>>> function makes defcustom super-awkward, and AFAIK, no other emacs core
>>>> setting does it this way.
>>>
>>> There's a bunch of "feature" setting that do not include full function
>>> names.
>>
>> ???
>
> I'm not sure what you're asking...

I guess he's asking what "feature setting" is.  (I'd like to know too)

Re: The netsec thread

[Jimmy Yuen Ho Wong]

On Mon, Jul 23, 2018 at 4:37 PM, Lars Ingebrigtsen <larsi@gnus.org> wrote:
> Jimmy Yuen Ho Wong <wyuenho@gmail.com> writes:
>
>> I want to make sure your need is taken cared of
>> properly (I'm contemplating an actual debug mode that prompts after
>> every handshake) without introducing something into emacs that's so
>> far only useful for one person.
>
> I feel I have to remind you that we're not introducing something; it's
> you who insist on removing something without any period of obsoleting
> the feature.
>

I don't know of any way of obsoleting a symbol other than docstring
and maybe NEWS. Removing it and aliasing 'paranoid to 'high is the
closest I can think of that doesn't break people's config. Obviously
this change should be in NEWS and maybe docstring, which I will get
to.

Re: The netsec thread

[Noam Postavsky]

On 23 July 2018 at 11:51, Jimmy Yuen Ho Wong <wyuenho@gmail.com> wrote:

> I don't know of any way of obsoleting a symbol other than docstring
> and maybe NEWS.

nsm-level could call display-warning if SYMBOL is `paranoid' (and then
disable the warning by adding to warning-suppress-log-types so as not
to bother the user more than once per session).

Re: The netsec thread

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

> On 23 July 2018 at 11:51, Jimmy Yuen Ho Wong <wyuenho@gmail.com> wrote:
>
>> I don't know of any way of obsoleting a symbol other than docstring
>> and maybe NEWS.
>
> nsm-level could call display-warning if SYMBOL is `paranoid' (and then
> disable the warning by adding to warning-suppress-log-types so as not
> to bother the user more than once per session).

Yup.  But since we're not obsoleting that value, it's a rather academic
discussion.

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

Re: The netsec thread

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

>>>> There's a bunch of "feature" setting that do not include full function
>>>> names.
>>>
>>> ???
>>
>> I'm not sure what you're asking...
>
> I guess he's asking what "feature setting" is.  (I'd like to know too)

I thought that was a common pattern (i.e., saying that you want features
`(foo bar baz)' for `foobarify-things'), but I grepped through the
defcustom settings and I couldn't find any, so perhaps I'm wrong.  Or
just bad with grep.  :-)

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

Re: The netsec thread

[Jimmy Yuen Ho Wong]

>>
>> That's exactly what it is, and why `network-trust-local-network`
>> defaults to nil. I can fix the docstrings.
>
> That may be its intent, but thatʼs not what the implementation in
> nsm.el does, as far as I can tell. All I see are comparisons to
> RFC1918 addresses.
>

Yes there's currently no way to define your own subnet range in this
iteration. I'm a bit afraid of over-engineering this. What are you
thoughts?

Re: The netsec thread

[Andy Moreton]

On Mon 23 Jul 2018, Lars Ingebrigtsen wrote:

> Andy Moreton <andrewjmoreton@gmail.com> writes:
>
>> You have not understood what I meant: the existing code is buggy. It is
>> supposed to allow the user to press a limited number of keys to decide
>> what to do with the presented certificate. however if you press any
>> other key, the NSM gets stuck and you have to quit with ^G to kill it.
>
> That sounds like a bug.  When I hit a different key it just says that
> it's an invalid key and I can then hit the right key.
>
> Do you have a test case?

I've seen this with gnus when reading blog articles from gmane.org,
where the article has several images, and this in init.el:

    (setq gnutls-algorithm-priority "normal")
    (setq gnutls-min-prime-bits nil)

    (require 'nsm)
    (setq network-security-level 'high)
    (setq nsm-save-host-names    t)

For some article it prompts for each image for a certificate. The
message in the echo erea obscures the next question in the minibuffer,
and any keypress other than the expeced ones seems to send it into a
busy loop that is only fixed by quitting with ^G.

This is on Windows 10 with 64bit emacs (mingw64 MSYS2) built from either
emacs-26 or master.

    AndyM

Re: The netsec thread

[Robert Pluim]

Jimmy Yuen Ho Wong <wyuenho@gmail.com> writes:

>>>
>>> That's exactly what it is, and why `network-trust-local-network`
>>> defaults to nil. I can fix the docstrings.
>>
>> That may be its intent, but thatʼs not what the implementation in
>> nsm.el does, as far as I can tell. All I see are comparisons to
>> RFC1918 addresses.
>>
>
> Yes there's currently no way to define your own subnet range in this
> iteration. I'm a bit afraid of over-engineering this. What are you
> thoughts?

Either you can allow the user to specify a specific subnet, or you
combine the results of something like

(map-apply (lambda (a b) (network-interface-info a))
(network-interface-list))

with (network-lookup-address-info host) where host is where youʼre
trying to connect to.

I still fail to see the real need for it, but I guess Eli has a
use-case.

Regards

Robert

Re: The netsec thread

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: Emacs-Devel devel <emacs-devel@gnu.org>,  Eli Zaretskii <eliz@gnu.org>
> Date: Mon, 23 Jul 2018 19:25:48 +0200
> 
> I still fail to see the real need for it, but I guess Eli has a
> use-case.

All the computers on my home network is my use case.

Re: The netsec thread

[Andy Moreton]

On Mon 23 Jul 2018, Andy Moreton wrote:

> On Mon 23 Jul 2018, Lars Ingebrigtsen wrote:
>
>> Andy Moreton <andrewjmoreton@gmail.com> writes:
>>
>>> You have not understood what I meant: the existing code is buggy. It is
>>> supposed to allow the user to press a limited number of keys to decide
>>> what to do with the presented certificate. however if you press any
>>> other key, the NSM gets stuck and you have to quit with ^G to kill it.
>>
>> That sounds like a bug.  When I hit a different key it just says that
>> it's an invalid key and I can then hit the right key.
>>
>> Do you have a test case?
>
> I've seen this with gnus when reading blog articles from gmane.org,
> where the article has several images, and this in init.el:

After a bit more testing I find that gnus can still be provoked into
getting stuck on the emacs-26 branch:

 - delete ~/.emacs.d/network-security.data and restart emacs
 - start gnus, and enter group gwene.electronics.righto on
 news.gmane.org
 - read one or more articles which contain more than one image
 - NSM prompts for a certificate from *.googleusercontent.com (name
 varies)
 - press keys otehr than the expected a/s/n/? (e.g. <up> <down> <left> <right>)
 - NSM gets stuck and never seems to discard the unexpected keys.
   only quitting with ^G seems to fix it.

In further testing I cannot reproduce this on master, so hopefully the
problem is gone :-)

    AndyM

Re: The netsec thread

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Robert Pluim <rpluim@gmail.com>
>> Cc: Emacs-Devel devel <emacs-devel@gnu.org>,  Eli Zaretskii <eliz@gnu.org>
>> Date: Mon, 23 Jul 2018 19:25:48 +0200
>> 
>> I still fail to see the real need for it, but I guess Eli has a
>> use-case.
>
> All the computers on my home network is my use case.

You regularly make connections from one machine running emacs to a
different machine on the same subnet using TLS? And you donʼt want to
answer 'a' once per target machine? Iʼll allow it, even if I donʼt
understand it. :-)

Trial implementation attached, based off the netsec branch. I got rid
of all the RFC 1918 stuff, and it allows localhost connections
regardless of the value of nsm-trust-local-network, which Iʼm also not
convinced about.

I do wonder if checking all of the target host's addresses is
overkill, but we can always take that out.

diff --git i/lisp/net/nsm.el w/lisp/net/nsm.el
index b59ea07d8a..682a238cc1 100644
--- i/lisp/net/nsm.el
+++ w/lisp/net/nsm.el
@@ -70,9 +70,15 @@ nsm-trust-local-network
 such as attempting to connect to an email server that do not
 follow these practices inside a school or corporate network, NSM
 may produce warnings for such occasions.  Setting this option to
-a non-nil value, or a zero-argument function that returns non-nil
-tells NSM to skip checking for potential TLS vulnerabilities when
-connecting to hosts on a local network.
+a non-nil value tells NSM to skip checking for potential TLS
+vulnerabilities when connecting to hosts on a local network,
+which is determined by inspecting the network addresses of the
+local machine.
+
+This option can be set to a function taking one argument, in
+which case that function will be called once for each IP address
+that is found for the target host.  It should return t if TLS
+checks should not be performed for that address.
 
 Make sure you know what you are doing before enabling this
 option."
@@ -204,54 +210,74 @@ nsm-tls-post-check-functions
 RESULTS is an alist where the keys are the checks run and the
 values the results of the checks.")
 
+(defun network-same-subnet-p (local-ip mask ip)
+  "Returns t if IP is in the same subnet as LOCAL-IP/MASK.
+LOCAL-IP, MASK, and IP are specified as vectors of integers, and
+are expected to have the same length.  Works for both IPv4 and
+IPv6 addresses."
+  (let ((matches t)
+        (length (length local-ip)))
+    (unless (memq length '(4 5 8 9))
+      (error "Unexpected length of IP address %S" local-ip))
+    (dotimes (i length)
+      (setq matches (and matches
+                         (=
+                          (logand (aref local-ip i)
+                                  (aref mask i))
+                          (logand (aref ip i)
+                                  (aref mask i))))))
+    matches))
+
 (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."
-  (let* ((address (or (nslookup-host-ipv4 host nil 'vector)
-                      (nslookup-host-ipv6 host nil 'vector)))
-         (ipv4? (eq (length address) 4)))
+If one of HOST's addresses is a localhost address this returns
+nil.
+
+If `nsm-trust-local-network' is t this function will check if any
+of HOST's addresses are in the same subnet as any directly
+connected interfaces, and will return nil if so.
+
+If `nsm-trust-local-network' is a function it will be called once
+for each address of HOST.  If one of those calls returns t then
+`nsm-should-check' will return nil immediately, and processing of
+the HOST's addresses will stop.
+
+Otherwise returns t."
+  (let ((addresses (network-lookup-address-info host)))
     (not
-     (or (if ipv4?
-             (or
-              ;; (0.x.x.x) this machine
-              (eq (aref address 0) 0)
-              ;; (127.x.x.x) localhost
-              (eq (aref address 0) 0))
-           (or
-            ;; (::) IPv6 this machine
-            (not (cl-mismatch address [0 0 0 0 0 0 0 0]))
-            ;; (::1) IPv6 localhost
-            (not (cl-mismatch address [0 0 0 0 0 0 0 1]))))
-         (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
-                   (and (eq (aref address 0) 192)
-                        (eq (aref address 0) 168))
-                   ;; (198.18.x.x) private
-                   (and (eq (aref address 0) 198)
-                        (eq (aref address 0) 18))
-                   ;; (169.254.x.x) link-local
-                   (and (eq (aref address 0) 169)
-                        (eq (aref address 0) 254)))
-                (memq (aref address 0)
-                      '(
-                        64512  ;; (fc00::) IPv6 unique local address
-                        64768  ;; (fd00::) IPv6 unique local address
-                        65152  ;; (fe80::) IPv6 link-local
-                        )
-                      )))))))
+     (catch 'trust
+       (dolist (address addresses)
+         (let* ((length (length address))
+                (ipv4? (eq length 4)))
+           (if ipv4?
+               ;; (127.x.x.x) localhost
+               (and (eq (aref address 0) 127)
+                    (throw 'trust t))
+             ;; (::) IPv6 this machine and (::1) IPv6 localhost
+             (and (or (not (cl-mismatch address [0 0 0 0 0 0 0 0]))
+                      (not (cl-mismatch address [0 0 0 0 0 0 0 1]))
+                      (memq (aref address 0)
+                            '(
+                              64512  ;; (fc00::) IPv6 unique local address
+                              64768  ;; (fd00::) IPv6 unique local address
+                              65152  ;; (fe80::) IPv6 link-local
+                              )))
+                  (throw 'trust t)))
+           (cond
+            ((functionp nsm-trust-local-network)
+             (and (funcall nsm-trust-local-network address)
+                  (throw 'trust t)))
+            (nsm-trust-local-network
+             (and
+              (cl-find-if #'(lambda (x)
+                              (network-same-subnet-p (subseq (car x) 0 length)
+                                                     (subseq (caddr x) 0 length)
+                                                     address))
+                          (map-apply (lambda (a b)
+                                       (network-interface-info a))
+                                     (network-interface-list)))
+              (throw 'trust t))))))))))
 
 (defun nsm-check-tls-connection (process host port status settings)
   "Check TLS connection against potential security problems.

Re: The netsec thread

[Lars Ingebrigtsen]

Andy Moreton <andrewjmoreton@gmail.com> writes:

>  - start gnus, and enter group gwene.electronics.righto on
>  news.gmane.org
>  - read one or more articles which contain more than one image
>  - NSM prompts for a certificate from *.googleusercontent.com (name
>  varies)
>  - press keys otehr than the expected a/s/n/? (e.g. <up> <down> <left> <right>)
>  - NSM gets stuck and never seems to discard the unexpected keys.
>    only quitting with ^G seems to fix it.

Oh, I see.  The NSM shouldn't really be firing at all, since this is
happening from a non-interactive-user context.  I.e., as originally
designed, these connections (for images fetched in the background) were
just meant to be closed and discarded.

That's the way it worked in Emacs 25, but perhaps the async network code
rework made this stop working.

> In further testing I cannot reproduce this on master, so hopefully the
> problem is gone :-)

And somehow it's started working again.  :-)

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

Re: The netsec thread

[Andy Moreton]

On Tue 24 Jul 2018, Lars Ingebrigtsen wrote:

> Andy Moreton <andrewjmoreton@gmail.com> writes:
>
>>  - start gnus, and enter group gwene.electronics.righto on
>>  news.gmane.org
>>  - read one or more articles which contain more than one image
>>  - NSM prompts for a certificate from *.googleusercontent.com (name
>>  varies)
>>  - press keys otehr than the expected a/s/n/? (e.g. <up> <down> <left> <right>)
>>  - NSM gets stuck and never seems to discard the unexpected keys.
>>    only quitting with ^G seems to fix it.
>
> Oh, I see.  The NSM shouldn't really be firing at all, since this is
> happening from a non-interactive-user context.  I.e., as originally
> designed, these connections (for images fetched in the background) were
> just meant to be closed and discarded.
>
> That's the way it worked in Emacs 25, but perhaps the async network code
> rework made this stop working.

I have now managed to reproduce this on master as well :-(

Using the same recipe as above, start gnus and enter the
gwene.org.haskell.planet group on gmane.org, and choose this post:

    From: Mark Dominus <post@gwene.org>
    Newsgroups: gwene.org.haskell.planet
    Subject: Operations that are not quite associative
    Date: Mon, 23 Jul 2018 18:57:00 +0200
    Message-ID: <x1-SeH1EAzsED7DgWAaBfpTmwM7acc@gwene.org>

When NSM starts prompting for certificate stuff, type some keys that are
not in its list of accepted options. It gets stuck in a loop, with the
echo area showing alternating lines of:

    Invalid choice.  Continue connecting ? (always, session only, no, ?): 
    Garbage collecting...

The garbage collection message is from these settings in init.el:
    (setq garbage-collection-messages t)
    (setq gc-cons-threshold  50000000)
    (setq gc-cons-percentage 0.3)

The "Invalid choice" mesage suggests it is getting stuck in
read-multiple-choice called from nsm-query-user. I'lll try to debug
further when I have more time.

    AndyM

Re: The netsec thread

[Andy Moreton]

On Tue 24 Jul 2018, Andy Moreton wrote:

> On Tue 24 Jul 2018, Lars Ingebrigtsen wrote:
>
>> Andy Moreton <andrewjmoreton@gmail.com> writes:
>>
>>>  - start gnus, and enter group gwene.electronics.righto on
>>>  news.gmane.org
>>>  - read one or more articles which contain more than one image
>>>  - NSM prompts for a certificate from *.googleusercontent.com (name
>>>  varies)
>>>  - press keys otehr than the expected a/s/n/? (e.g. <up> <down> <left> <right>)
>>>  - NSM gets stuck and never seems to discard the unexpected keys.
>>>    only quitting with ^G seems to fix it.
>>
>> Oh, I see.  The NSM shouldn't really be firing at all, since this is
>> happening from a non-interactive-user context.  I.e., as originally
>> designed, these connections (for images fetched in the background) were
>> just meant to be closed and discarded.
>>
>> That's the way it worked in Emacs 25, but perhaps the async network code
>> rework made this stop working.
>
> I have now managed to reproduce this on master as well :-(

A simpler recipe to reproduce this on master:

 - delete ~/.emacs.d/network-security.data
 - start emacs and then M-x gnus
 - NSM prompts for "Continue connecting ?"
 - mouse click in the "*Network Security Manager*" window
 - NSM repeats the prompt endlessly (along with garbage collection
   messages), and ignores all input except ^G to quit.

After quitting with ^G 3 times (multiple servers in gnus), I see the
following in *Messages*:

    Reading c:/home/ajm/.emacs.d/gnus/.newsrc.eld...
    Checking new news...
    Reading active file via nnnil...done
    Reading active file from cache via nnml...
    Opening nnml server on cache...done
    Reading active file from cache via nnml...done
    Type "q" to delete help window.
    Continue connecting? (always, session only, no, ?):  [116582 times]
    Aborting connection to news.eternal-september.org:nntp
    Type "q" to delete help window.
    Continue connecting? (always, session only, no, ?):  [13598 times]
    Aborting connection to news.gmane.org:nntp
    Type "q" to delete help window.
    Continue connecting? (always, session only, no, ?):  [23369 times]
    Aborting connection to feedbase.org:563
    Reading active file via nndraft...done
    Checking new news...done

Something is broken in read-multiple-choice if it shows "Continue
connecting?" that many times (also hinted at by the triggering of
frequent garbage collections).

    AndyM

Re: The netsec thread

[Noam Postavsky]

On 24 July 2018 at 07:54, Andy Moreton <andrewjmoreton@gmail.com> wrote:

> Something is broken in read-multiple-choice if it shows "Continue
> connecting?" that many times (also hinted at by the triggering of
> frequent garbage collections).

I've opened Bug#32257 for this.

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=32257

Re: The netsec thread

[Jimmy Yuen Ho Wong]

On Tue, Jul 24, 2018 at 1:09 PM, Noam Postavsky <npostavs@gmail.com> wrote:
> On 24 July 2018 at 07:54, Andy Moreton <andrewjmoreton@gmail.com> wrote:
>
>> Something is broken in read-multiple-choice if it shows "Continue
>> connecting?" that many times (also hinted at by the triggering of
>> frequent garbage collections).
>
> I've opened Bug#32257 for this.
>
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=32257
>

Regardless of the issue in rmc, NSM probably shouldn't even pop up a
modal in certain applications like Gnus and EWW when one document may
trigger multiple TLS connections. This is where we should take a page
from the browsers and email clients. For Gnus, introduce an option to
stop the downloading of images in HTML emails, so the sender cannot
track you. For email and EWW, if the TLS connections to load the
assets of the same email or webpage are insecure, do not even prompt,
but put something like [insecure content] in the mode line.

Re: The netsec thread

[Lars Ingebrigtsen]

Jimmy Yuen Ho Wong <wyuenho@gmail.com> writes:

> This is where we should take a page from the browsers and email
> clients. For Gnus, introduce an option to stop the downloading of
> images in HTML emails, so the sender cannot track you.

Gnus inhibits loading external resources in emails, of course.

> For email and EWW, if the TLS connections to load the assets of the
> same email or webpage are insecure, do not even prompt,

This is what used to happen in Emacs 25, but like I said, it's
apparently stopped working at some point (which leads to the prompting).

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

Re: The netsec thread

[Andy Moreton]

On Tue 24 Jul 2018, Jimmy Yuen Ho Wong wrote:

> On Tue, Jul 24, 2018 at 1:09 PM, Noam Postavsky <npostavs@gmail.com> wrote:
>> On 24 July 2018 at 07:54, Andy Moreton <andrewjmoreton@gmail.com> wrote:
>>
>>> Something is broken in read-multiple-choice if it shows "Continue
>>> connecting?" that many times (also hinted at by the triggering of
>>> frequent garbage collections).
>>
>> I've opened Bug#32257 for this.
>>
>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=32257
>>
>
> Regardless of the issue in rmc, NSM probably shouldn't even pop up a
> modal in certain applications like Gnus and EWW when one document may
> trigger multiple TLS connections. This is where we should take a page
> from the browsers and email clients. For Gnus, introduce an option to
> stop the downloading of images in HTML emails, so the sender cannot
> track you. For email and EWW, if the TLS connections to load the
> assets of the same email or webpage are insecure, do not even prompt,
> but put something like [insecure content] in the mode line.

No, give people the option to control which sites are accessed at all,
for privacy as well as security (like an ad-blocker).

    AndyM

Re: The netsec thread

[Lars Ingebrigtsen]

I think we kinda let this issue fall off the radar for (looks at looks
at calendar) about a year, but I think we should definitely merge the
extended NSM checks before Emacs 27 is released (the process of which is
apparently tentatively scheduled to start on a month or two?).

I've skimmed the different threads and bug reports (bug#31946 being the
primary one), but I'm not sure what the status ended up being.  I
vaguely thought I remembered we were waiting for copyright paperwork,
but that happened in July last year, so that should be fine.

Jimmy, did you have a final patch prepared for the NSM that we just
forgot to apply, or were you going to do more work on it?

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

Re: The netsec thread

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sun, 28 Jul 2019 20:18:12 +0200
> Cc: Andy Moreton <andrewjmoreton@gmail.com>,
>  Noam Postavsky <npostavs@gmail.com>, Emacs developers <emacs-devel@gnu.org>
> 
> Jimmy, did you have a final patch prepared for the NSM that we just
> forgot to apply, or were you going to do more work on it?

Isn't the stuff sitting on the netsec branch?

Re: The netsec thread

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Lars Ingebrigtsen <larsi@gnus.org>
>> Date: Sun, 28 Jul 2019 20:18:12 +0200
>> Cc: Andy Moreton <andrewjmoreton@gmail.com>,
>>  Noam Postavsky <npostavs@gmail.com>, Emacs developers <emacs-devel@gnu.org>
>> 
>> Jimmy, did you have a final patch prepared for the NSM that we just
>> forgot to apply, or were you going to do more work on it?
>
> Isn't the stuff sitting on the netsec branch?

So it is.  Last checkin was July 17th by Robert Pluim.

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

Re: The netsec thread

[Lars Ingebrigtsen]

July 17th 2018, that is.

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

Re: The netsec thread

[Lars Ingebrigtsen]

I've now done some testing of the netsec branch, and it basically looks
good to me.  It's a bit too detailed in the warnings it presents to the
user -- the original idea was to keep the level of detail down so that
it won't scare away everyone but security professionals, and it's now
rather scary.

I've only skimmed the patch set -- it's 2200 lines, but I've got one
question to Robert: The patches that add `network-lookup-address-info'
went into the netsec branch.  Was there any particular reason for that?
They seem rather unrelated.  (It does look like a useful addition,
though.)

So my plan here is to wait a few days to see whether there are any
further comments, and then merge the branch into the trunk.  I will then
do some cosmetic touch-ups; mostly moving all new details displayed on
the first warning screen to the "details" page.

Sound OK to everybody?

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

Re: The netsec thread

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sun, 28 Jul 2019 21:08:05 +0200
> Cc: Robert Pluim <rpluim@gmail.com>, emacs-devel@gnu.org,
>  andrewjmoreton@gmail.com, wyuenho@gmail.com, npostavs@gmail.com
> 
> So my plan here is to wait a few days to see whether there are any
> further comments, and then merge the branch into the trunk.  I will then
> do some cosmetic touch-ups; mostly moving all new details displayed on
> the first warning screen to the "details" page.
> 
> Sound OK to everybody?

Please be sure all the changes in behavior, both user-level and
Lisp-level, are called out in NEWS.  Currently, none of that is.

Thanks.

Re: The netsec thread

[Robert Pluim]

>>>>> On Sun, 28 Jul 2019 21:08:05 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:

    Lars> I've now done some testing of the netsec branch, and it basically looks
    Lars> good to me.  It's a bit too detailed in the warnings it presents to the
    Lars> user -- the original idea was to keep the level of detail down so that
    Lars> it won't scare away everyone but security professionals, and it's now
    Lars> rather scary.

    Lars> I've only skimmed the patch set -- it's 2200 lines, but I've got one
    Lars> question to Robert: The patches that add `network-lookup-address-info'
    Lars> went into the netsec branch.  Was there any particular reason for that?
    Lars> They seem rather unrelated.  (It does look like a useful addition,
    Lars> though.)

I seem to remember Jimmy wanted it so he could add further consistency
checks. He dropped off before he could explain exactly what those
were, and they're not necessary for his changes.

    Lars> So my plan here is to wait a few days to see whether there are any
    Lars> further comments, and then merge the branch into the trunk.  I will then
    Lars> do some cosmetic touch-ups; mostly moving all new details displayed on
    Lars> the first warning screen to the "details" page.

I had some issues with nsm-trust-local-network as a concept and also
the IP addresses it checked. And 0.0.0.0/8 is now a valid range (on
very recent Linux kernels anyway), so that test needs adjusting.

Did I send a patch for that? I donʼt remember, and Monday morning
laziness is strong today.

    Lars> Sound OK to everybody?

Sure. Please leave network-security-level 'paranoid alone, though.

Robert

Re: The netsec thread

[Robert Pluim]

>>>>> On Mon, 29 Jul 2019 09:50:03 +0200, Robert Pluim <rpluim@gmail.com> said:

>>>>> On Sun, 28 Jul 2019 21:08:05 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:
    Lars> I've now done some testing of the netsec branch, and it basically looks
    Lars> good to me.  It's a bit too detailed in the warnings it presents to the
    Lars> user -- the original idea was to keep the level of detail down so that
    Lars> it won't scare away everyone but security professionals, and it's now
    Lars> rather scary.

    Lars> I've only skimmed the patch set -- it's 2200 lines, but I've got one
    Lars> question to Robert: The patches that add `network-lookup-address-info'
    Lars> went into the netsec branch.  Was there any particular reason for that?
    Lars> They seem rather unrelated.  (It does look like a useful addition,
    Lars> though.)

    Robert> I seem to remember Jimmy wanted it so he could add further consistency
    Robert> checks. He dropped off before he could explain exactly what those
    Robert> were, and they're not necessary for his changes.

Although you could use it to replace his nslookup-host-ipv{4,6}
functions.

Robert

Re: The netsec thread

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Please be sure all the changes in behavior, both user-level and
> Lisp-level, are called out in NEWS.  Currently, none of that is.

Yes, of course.

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

Re: The netsec thread

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

> I had some issues with nsm-trust-local-network as a concept and also
> the IP addresses it checked. And 0.0.0.0/8 is now a valid range (on
> very recent Linux kernels anyway), so that test needs adjusting.

Oh, yeah, I had forgotten about that bit.  Yes, I wasn't convinced about
that bit, either.

> Did I send a patch for that? I donʼt remember, and Monday morning
> laziness is strong today.

Let's see...  You didn't push it to the netsec branch, at least.  If you
manage to find the patch, please do.

> Sure. Please leave network-security-level 'paranoid alone, though.

That's another bit I'd forgotten about; thanks.  Yes, we want to leave
that as is.

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

Re: The netsec thread

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

>     Robert> I seem to remember Jimmy wanted it so he could add further
>     Robert> consistency
>     Robert> checks. He dropped off before he could explain exactly what those
>     Robert> were, and they're not necessary for his changes.
>
> Although you could use it to replace his nslookup-host-ipv{4,6}
> functions.

Ah, yes, good idea.  Using the external nslookup program wouldn't work
on all platforms anyway.

OK, so it looks like the branch needs more work than skimming through
the patch indicated, so I won't be merging in a couple of days, anyway.

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

Re: The netsec thread

[Robert Pluim]

>>>>> On Mon, 29 Jul 2019 13:14:44 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:

    Lars> Robert Pluim <rpluim@gmail.com> writes:
    >> I had some issues with nsm-trust-local-network as a concept and also
    >> the IP addresses it checked. And 0.0.0.0/8 is now a valid range (on
    >> very recent Linux kernels anyway), so that test needs adjusting.

    Lars> Oh, yeah, I had forgotten about that bit.  Yes, I wasn't convinced about
    Lars> that bit, either.

    >> Did I send a patch for that? I donʼt remember, and Monday morning
    >> laziness is strong today.

    Lars> Let's see...  You didn't push it to the netsec branch, at least.  If you
    Lars> manage to find the patch, please do.

I found the bottom half of the implementation, and just wrote the top
half.  Iʼm wondering if 'network-lookup-address-info' should just
return 5/9 element vectors like 'network-interface-info' (for
IPv4/IPv6), so that remembering to chop the port off the end becomes
unnecessary.

Tested only on macOS so far. It covers the localhost case because
'network-interface-list' returns the loopback interface, which is true
on my GNU/Linux box as well. If thereʼs a platform where thatʼs not
true we'll have to adjust the test.

Robert

2019-07-29  Robert Pluim  <rpluim@gmail.com>

	* lisp/net/nsm.el (nsm-network-same-subnet): New function.  Checks
	if an ip address is in the same subnet as another one.
	(nsm-should-check): Use nsm-network-same-subnet to see if we're
	connecting to a local subnet machine.  Remove checks for RFC1918 addresses.


diff --git i/lisp/net/nsm.el w/lisp/net/nsm.el
index b59ea07d8a..3adc1b1dc5 100644
--- i/lisp/net/nsm.el
+++ w/lisp/net/nsm.el
@@ -204,54 +204,52 @@ nsm-tls-post-check-functions
 RESULTS is an alist where the keys are the checks run and the
 values the results of the checks.")
 
+(defun nsm-network-same-subnet (local-ip mask ip)
+  "Returns t if IP is in the same subnet as LOCAL-IP/MASK.
+LOCAL-IP, MASK, and IP are specified as vectors of integers, and
+are expected to have the same length.  Works for both IPv4 and
+IPv6 addresses."
+  (let ((matches t)
+        (length (length local-ip)))
+    (unless (memq length '(4 5 8 9))
+      (error "Unexpected length of IP address %S" local-ip))
+    (dotimes (i length)
+      (setq matches (and matches
+                         (=
+                          (logand (aref local-ip i)
+                                  (aref mask i))
+                          (logand (aref ip i)
+                                  (aref mask i))))))
+    matches))
+
+
 (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."
-  (let* ((address (or (nslookup-host-ipv4 host nil 'vector)
-                      (nslookup-host-ipv6 host nil 'vector)))
-         (ipv4? (eq (length address) 4)))
-    (not
-     (or (if ipv4?
-             (or
-              ;; (0.x.x.x) this machine
-              (eq (aref address 0) 0)
-              ;; (127.x.x.x) localhost
-              (eq (aref address 0) 0))
-           (or
-            ;; (::) IPv6 this machine
-            (not (cl-mismatch address [0 0 0 0 0 0 0 0]))
-            ;; (::1) IPv6 localhost
-            (not (cl-mismatch address [0 0 0 0 0 0 0 1]))))
-         (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
-                   (and (eq (aref address 0) 192)
-                        (eq (aref address 0) 168))
-                   ;; (198.18.x.x) private
-                   (and (eq (aref address 0) 198)
-                        (eq (aref address 0) 18))
-                   ;; (169.254.x.x) link-local
-                   (and (eq (aref address 0) 169)
-                        (eq (aref address 0) 254)))
-                (memq (aref address 0)
-                      '(
-                        64512  ;; (fc00::) IPv6 unique local address
-                        64768  ;; (fd00::) IPv6 unique local address
-                        65152  ;; (fe80::) IPv6 link-local
-                        )
-                      )))))))
+host address is a localhost address, or in the same subnet as one
+of the local interfaces, this function returns nil.  Non-nil
+otherwise."
+  (let ((addresses (network-lookup-address-info host))
+        (network-interface-list (network-interface-list))
+        (off-net t))
+    (when
+     (or (and (functionp nsm-trust-local-network)
+              (funcall nsm-trust-local-network))
+         nsm-trust-local-network)
+     (mapc
+      (lambda (address)
+        (mapc
+         (lambda (iface)
+           (let ((info (network-interface-info (car iface))))
+             (when
+                 (nsm-network-same-subnet (substring (car info) 0 -1)
+                                          (substring (car (cddr info)) 0 -1)
+                                          address)
+               (setq off-net nil))))
+         network-interface-list))
+      addresses))
+     off-net))
 
 (defun nsm-check-tls-connection (process host port status settings)
   "Check TLS connection against potential security problems.

Re: The netsec thread

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

> I found the bottom half of the implementation, and just wrote the top
> half.  Iʼm wondering if 'network-lookup-address-info' should just
> return 5/9 element vectors like 'network-interface-info' (for
> IPv4/IPv6), so that remembering to chop the port off the end becomes
> unnecessary.

Makes sense, I think.

> Tested only on macOS so far. It covers the localhost case because
> 'network-interface-list' returns the loopback interface, which is true
> on my GNU/Linux box as well. If thereʼs a platform where thatʼs not
> true we'll have to adjust the test.
>
> Robert
>
> 2019-07-29  Robert Pluim  <rpluim@gmail.com>
>
> 	* lisp/net/nsm.el (nsm-network-same-subnet): New function.  Checks
> 	if an ip address is in the same subnet as another one.
> 	(nsm-should-check): Use nsm-network-same-subnet to see if we're
> 	connecting to a local subnet machine.  Remove checks for RFC1918 addresses.

OK; are you applying this to the netsec branch?

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

Re: The netsec thread

[Robert Pluim]

>>>>> On Tue, 30 Jul 2019 13:30:56 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:

    Lars> Robert Pluim <rpluim@gmail.com> writes:
    >> I found the bottom half of the implementation, and just wrote the top
    >> half.  Iʼm wondering if 'network-lookup-address-info' should just
    >> return 5/9 element vectors like 'network-interface-info' (for
    >> IPv4/IPv6), so that remembering to chop the port off the end becomes
    >> unnecessary.

    Lars> Makes sense, I think.

    >> Tested only on macOS so far. It covers the localhost case because
    >> 'network-interface-list' returns the loopback interface, which is true
    >> on my GNU/Linux box as well. If thereʼs a platform where thatʼs not
    >> true we'll have to adjust the test.
    >> 
    >> Robert
    >> 
    >> 2019-07-29  Robert Pluim  <rpluim@gmail.com>
    >> 
    >> * lisp/net/nsm.el (nsm-network-same-subnet): New function.  Checks
    >> if an ip address is in the same subnet as another one.
    >> (nsm-should-check): Use nsm-network-same-subnet to see if we're
    >> connecting to a local subnet machine.  Remove checks for RFC1918 addresses.

    Lars> OK; are you applying this to the netsec branch?

Yes, once Iʼve finished writing the tests, and undone the port
stripping in network-lookup-address-info, and made sure it works for
ipv6. My home internet is down at the moment, so it might be a few
days.

Robert

Re: The netsec thread

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

> Yes, once Iʼve finished writing the tests, and undone the port
> stripping in network-lookup-address-info, and made sure it works for
> ipv6. My home internet is down at the moment, so it might be a few
> days.

Okidoke.  I could start looking at tweaking some of the other behaviour
you mentioned (paranoid etc) in the meantime, and then we could
tentatively aim at having something ready for merging in...  a couple of
weeks?

I'm going away on a holiday, though, so the timeline may have to be
pushed a bit (unless somebody else fixes those things, too).

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

Re: The netsec thread

[Robert Pluim]

>>>>> On Tue, 30 Jul 2019 15:32:13 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:

    Lars> Robert Pluim <rpluim@gmail.com> writes:
    >> Yes, once Iʼve finished writing the tests, and undone the port
    >> stripping in network-lookup-address-info, and made sure it works for
    >> ipv6. My home internet is down at the moment, so it might be a few
    >> days.

    Lars> Okidoke.  I could start looking at tweaking some of the other behaviour
    Lars> you mentioned (paranoid etc) in the meantime, and then we could
    Lars> tentatively aim at having something ready for merging in...  a couple of
    Lars> weeks?

Sure. Iʼm on vacation soon, which will give me more time to look at
this.

    Lars> I'm going away on a holiday, though, so the timeline may have to be
    Lars> pushed a bit (unless somebody else fixes those things, too).

I think itʼs just you and me :-)

Robert

Re: The netsec thread

[Robert Pluim]

>>>>> On Tue, 30 Jul 2019 17:05:46 +0200, Robert Pluim <rpluim@gmail.com> said:

>>>>> On Tue, 30 Jul 2019 15:32:13 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:
    Lars> Robert Pluim <rpluim@gmail.com> writes:
    >>> Yes, once Iʼve finished writing the tests, and undone the port
    >>> stripping in network-lookup-address-info, and made sure it works for
    >>> ipv6. My home internet is down at the moment, so it might be a few
    >>> days.

    Lars> Okidoke.  I could start looking at tweaking some of the other behaviour
    Lars> you mentioned (paranoid etc) in the meantime, and then we could
    Lars> tentatively aim at having something ready for merging in...  a couple of
    Lars> weeks?

    Robert> Sure. Iʼm on vacation soon, which will give me more time to look at
    Robert> this.

Iʼve just pushed it to the netsec branch, which I didnʼt rebase. The
nsm-should-check stuff will only cause emacs to skip checks to local
IPv4 subnets if itʼs enabled, but thatʼs because
'network-interface-info' only returns the first address it finds for
each interface, which is invariably IPv4. On platforms with
'getifaddrs' or the equivalent, we could extend it to

(network-interface-info ifname &optional addr-type)

Where addr-type is 'ipv4, 'ipv6, 'all

And have 'all mean return a list of address information element lists,
rather than a single one. But I donʼt have time to work on that right
now (and I have no idea what the w32 implementation would look like).

Robert

Re: The netsec thread

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

> On platforms with 'getifaddrs' or the equivalent, we could extend it
> to
>
> (network-interface-info ifname &optional addr-type)
>
> Where addr-type is 'ipv4, 'ipv6, 'all
>
> And have 'all mean return a list of address information element lists,
> rather than a single one.

I think that sounds like a good idea.

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

Re: The netsec thread

[Lars Ingebrigtsen]

I'm now back from holiday, so I fixed up the new NSM code and merged
it.  (I hope I didn't mess things up too badly during the merge -- there
were several conflicts.)

I've tried the resulting code, and everything seems to work OK to me,
but please let me know whether you see any new TLS-related problems
while browsing or something.

I'm not quite sure what to write in the NEWS file beyond what's already
there (that the security has been tightened, but that was already the
case last year :-/), and now it's just slightly more tightened.

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

Re: The netsec thread

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 821 bytes --]

Lars Ingebrigtsen wrote:
> I've tried the resulting code, and everything seems to work OK to me,
> but please let me know whether you see any new TLS-related problems
> while browsing or something.

I had a problem building it on Fedora 30 which uses GnuTLS 3.6.8, because 
starting in GnuTLS 3.6 the functions gnutls_compression_get and 
gnutls_compression_get_name are deprecated and cause compile-time errors when 
one builds with --enable-gcc-warnings. If you override the compile-time errors 
the compatibility stubs in libgnutls return null values, and some 
network-stream-tests fail.

I temporarily worked around the build problem by installing the attached patch 
which omits calls to these functions in GnuTLS 3.6 and later, but this doesn't 
fix the runtime issues. Is that something you could take a look at?

[-- Attachment #2: 0001-Get-the-Gnutls-code-compiling-on-Fedora-30.patch --]
[-- Type: text/x-patch, Size: 9119 bytes --]

From 49a8c8506a8477fd27ba924f14aa196e0d0813f9 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 23 Aug 2019 01:11:12 -0700
Subject: [PATCH] Get the Gnutls code compiling on Fedora 30

The recent changes caused the build to fail on Fedora 30 when built
with --enable-gcc-warnings, among other things with diagnostics that
gnutls_compression_get and gnutls_compression_get_name are deprecated
(this started with GnuTLS 3.6).  Fix this by refusing to call these
obsolescent and now-dummy functions in GnuTLS 3.6 and later.  However,
this is just a temporary workaround to get the build working; a real
fix is needed, as network-stream-tests fail.
* src/gnutls.c (HAVE_GNUTLS_COMPRESSION_GET): New macro.
(gnutls_compression_get, gnutls_compression_get_name):
Define only if HAVE_GNUTLS_COMPRESSION_GET.
(init_gnutls_functions): Load the two functions only if
HAVE_GNUTLS_COMPRESSION_GET.
(emacs_gnutls_certificate_export_pem): Use alloca instead of xmalloc.
(Fgnutls_peer_status): Just return "NULL" if the functions
are deprecated.
(Fgnutls_format_certificate): Fix pointer signedness glitches.
* src/process.c: Fix spacing.
---
 src/gnutls.c  | 60 +++++++++++++++++++++++++++++++--------------------
 src/process.c | 26 ++++++++++------------
 2 files changed, 48 insertions(+), 38 deletions(-)

diff --git a/src/gnutls.c b/src/gnutls.c
index db452e01aa..51536b1463 100644
--- a/src/gnutls.c
+++ b/src/gnutls.c
@@ -48,6 +48,10 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 # define HAVE_GNUTLS_ETM_STATUS
 #endif
 
+#if GNUTLS_VERSION_NUMBER < 0x030600
+# define HAVE_GNUTLS_COMPRESSION_GET
+#endif
+
 /* gnutls_mac_get_nonce_size was added in GnuTLS 3.2.0, but was
    exported only since 3.3.0. */
 #if GNUTLS_VERSION_NUMBER >= 0x030300
@@ -217,10 +221,12 @@ DEF_DLL_FN (const char *, gnutls_cipher_get_name,
 	    (gnutls_cipher_algorithm_t));
 DEF_DLL_FN (gnutls_mac_algorithm_t, gnutls_mac_get, (gnutls_session_t));
 DEF_DLL_FN (const char *, gnutls_mac_get_name, (gnutls_mac_algorithm_t));
+#ifdef HAVE_GNUTLS_COMPRESSION_GET
 DEF_DLL_FN (gnutls_compression_method_t, gnutls_compression_get,
             (gnutls_session_t));
 DEF_DLL_FN (const char *, gnutls_compression_get_name,
             (gnutls_compression_method_t));
+#endif
 DEF_DLL_FN (unsigned, gnutls_safe_renegotiation_status, (gnutls_session_t));
 
 #  ifdef HAVE_GNUTLS3
@@ -368,8 +374,10 @@ init_gnutls_functions (void)
   LOAD_DLL_FN (library, gnutls_cipher_get_name);
   LOAD_DLL_FN (library, gnutls_mac_get);
   LOAD_DLL_FN (library, gnutls_mac_get_name);
+#  ifdef HAVE_GNUTLS_COMPRESSION_GET
   LOAD_DLL_FN (library, gnutls_compression_get);
   LOAD_DLL_FN (library, gnutls_compression_get_name);
+#  endif
   LOAD_DLL_FN (library, gnutls_safe_renegotiation_status);
 #  ifdef HAVE_GNUTLS3
   LOAD_DLL_FN (library, gnutls_rnd);
@@ -462,8 +470,10 @@ init_gnutls_functions (void)
 #  define gnutls_kx_get_name fn_gnutls_kx_get_name
 #  define gnutls_mac_get fn_gnutls_mac_get
 #  define gnutls_mac_get_name fn_gnutls_mac_get_name
-#  define gnutls_compression_get fn_gnutls_compression_get
-#  define gnutls_compression_get_name fn_gnutls_compression_get_name
+#  ifdef HAVE_GNUTLS_COMPRESSION_GET
+#   define gnutls_compression_get fn_gnutls_compression_get
+#   define gnutls_compression_get_name fn_gnutls_compression_get_name
+#  endif
 #  define gnutls_safe_renegotiation_status fn_gnutls_safe_renegotiation_status
 #  define gnutls_pk_algorithm_get_name fn_gnutls_pk_algorithm_get_name
 #  define gnutls_pk_bits_to_sec_param fn_gnutls_pk_bits_to_sec_param
@@ -1082,17 +1092,18 @@ emacs_gnutls_certificate_export_pem (gnutls_x509_crt_t cert)
 
   if (err == GNUTLS_E_SHORT_MEMORY_BUFFER)
     {
-      unsigned char *buf = xmalloc(size * sizeof (unsigned char));
+      USE_SAFE_ALLOCA;
+      char *buf = SAFE_ALLOCA (size);
       err = gnutls_x509_crt_export (cert, GNUTLS_X509_FMT_PEM, buf, &size);
       check_memory_full (err);
 
       if (err < GNUTLS_E_SUCCESS)
-        {
-          xfree (buf);
-          error ("GnuTLS certificate export error: %s", emacs_gnutls_strerror (err));
-        }
+	error ("GnuTLS certificate export error: %s",
+	       emacs_gnutls_strerror (err));
 
-      return build_string(buf);
+      Lisp_Object result = build_string (buf);
+      SAFE_FREE ();
+      return result;
     }
   else if (err < GNUTLS_E_SUCCESS)
     error ("GnuTLS certificate export error: %s", emacs_gnutls_strerror (err));
@@ -1481,20 +1492,21 @@ returned as the :certificate entry.  */)
 				  (gnutls_mac_get (state)))));
 
   /* Compression name. */
-  result = nconc2
-    (result, list2 (intern (":compression"),
-		    build_string (gnutls_compression_get_name
-				  (gnutls_compression_get (state)))));
+#ifdef HAVE_GNUTLS_COMPRESSION_GET
+  Lisp_Object compression = build_string (gnutls_compression_get_name
+					  (gnutls_compression_get (state)));
+#else
+  Lisp_Object compression = build_string ("NULL");
+#endif
+  result = nconc2 (result, list2 (intern (":compression"), compression));
 
   /* Encrypt-then-MAC. */
-  result = nconc2
-    (result, list2 (intern (":encrypt-then-mac"),
+  Lisp_Object etm_status = Qnil;
 #ifdef HAVE_GNUTLS_ETM_STATUS
-                    gnutls_session_etm_status (state) ? Qt : Qnil
-#else
-                    Qnil
+  if (gnutls_session_etm_status (state))
+    etm_status = Qt;
 #endif
-                    ));
+  result = nconc2 (result, list2 (intern (":encrypt-then-mac"), etm_status));
 
   /* Renegotiation Indication */
   result = nconc2
@@ -1561,7 +1573,8 @@ boot_error (struct Lisp_Process *p, const char *m, ...)
   va_end (ap);
 }
 
-DEFUN ("gnutls-format-certificate", Fgnutls_format_certificate, Sgnutls_format_certificate, 1, 1, 0,
+DEFUN ("gnutls-format-certificate", Fgnutls_format_certificate,
+       Sgnutls_format_certificate, 1, 1, 0,
        doc: /* Format a X.509 certificate to a string.
 
 Given a PEM-encoded X.509 certificate CERT, returns a human-readable
@@ -1578,14 +1591,14 @@ string representation.  */)
   if (err < GNUTLS_E_SUCCESS)
     error ("gnutls-format-certificate error: %s", emacs_gnutls_strerror (err));
 
-  unsigned char *crt_buf = SDATA (cert);
-  gnutls_datum_t crt_data = { crt_buf, strlen (crt_buf) };
+  gnutls_datum_t crt_data = { SDATA (cert), strlen (SSDATA (cert)) };
   err = gnutls_x509_crt_import (crt, &crt_data, GNUTLS_X509_FMT_PEM);
   check_memory_full (err);
   if (err < GNUTLS_E_SUCCESS)
     {
       gnutls_x509_crt_deinit (crt);
-      error ("gnutls-format-certificate error: %s", emacs_gnutls_strerror (err));
+      error ("gnutls-format-certificate error: %s",
+	     emacs_gnutls_strerror (err));
     }
 
   gnutls_datum_t out;
@@ -1594,7 +1607,8 @@ string representation.  */)
   if (err < GNUTLS_E_SUCCESS)
     {
       gnutls_x509_crt_deinit (crt);
-      error ("gnutls-format-certificate error: %s", emacs_gnutls_strerror (err));
+      error ("gnutls-format-certificate error: %s",
+	     emacs_gnutls_strerror (err));
     }
 
   char *out_buf = xmalloc ((out.size + 1) * sizeof (char));
diff --git a/src/process.c b/src/process.c
index 7097b7ace1..c3cc78afa2 100644
--- a/src/process.c
+++ b/src/process.c
@@ -4120,10 +4120,8 @@ usage: (make-network-process &rest ARGS)  */)
       hints.ai_socktype = socktype;
 
       msg = network_lookup_address_info_1 (host, portstring, &hints, &res);
-      if (!EQ(msg, Qt))
-        {
-          error ("%s", SSDATA (msg));
-        }
+      if (!EQ (msg, Qt))
+	error ("%s", SSDATA (msg));
 
       for (lres = res; lres; lres = lres->ai_next)
 	addrinfos = Fcons (conv_addrinfo_to_lisp (lres), addrinfos);
@@ -4593,10 +4591,12 @@ network_lookup_address_info_1 (Lisp_Object host, const char *service,
         str = SSDATA (code_convert_string_norecord
                       (build_string (str), Vlocale_coding_system, 0));
       AUTO_STRING (format, "%s/%s %s");
-      msg = CALLN (Fformat, format, host, build_string (service), build_string (str));
+      msg = CALLN (Fformat, format, host, build_string (service),
+		   build_string (str));
 #else
       AUTO_STRING (format, "%s/%s getaddrinfo error %d");
-      msg = CALLN (Fformat, format, host, build_string (service), make_number (ret));
+      msg = CALLN (Fformat, format, host, build_string (service),
+		   make_number (ret));
 #endif
     }
    return msg;
@@ -4634,18 +4634,14 @@ nil if none were found.  Each address is a vector of integers.  */)
   hints.ai_socktype = SOCK_DGRAM;
 
   msg = network_lookup_address_info_1 (name, NULL, &hints, &res);
-  if (!EQ(msg, Qt))
-    {
-      message ("%s", SSDATA(msg));
-    }
+  if (!EQ (msg, Qt))
+    message ("%s", SSDATA(msg));
   else
     {
       for (lres = res; lres; lres = lres->ai_next)
-        {
-          addresses = Fcons (conv_sockaddr_to_lisp
-                             (lres->ai_addr, lres->ai_addrlen),
-                             addresses);
-        }
+	addresses = Fcons (conv_sockaddr_to_lisp (lres->ai_addr,
+						  lres->ai_addrlen),
+			   addresses);
       addresses = Fnreverse (addresses);
 
       freeaddrinfo (res);
-- 
2.17.1

Re: The netsec thread

[Lars Ingebrigtsen]

Paul Eggert <eggert@cs.ucla.edu> writes:

> I had a problem building it on Fedora 30 which uses GnuTLS 3.6.8,
> because starting in GnuTLS 3.6 the functions gnutls_compression_get
> and gnutls_compression_get_name are deprecated and cause compile-time
> errors when one builds with --enable-gcc-warnings. If you override the
> compile-time errors the compatibility stubs in libgnutls return null
> values, and some network-stream-tests fail.

Hm...  Since GnuTLS no longer supports compression at all, I would have
assumed that on newer OS versions this would be a NOOP.

> I temporarily worked around the build problem by installing the
> attached patch which omits calls to these functions in GnuTLS 3.6 and
> later, but this doesn't fix the runtime issues. Is that something you
> could take a look at?

Will do.

Hm...  Ah, I see that most of the network-stream tests fail now.  I
should have checked that out before pushing the merge; sorry.

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

Re: The netsec thread

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Hm...  Ah, I see that most of the network-stream tests fail now.  I
> should have checked that out before pushing the merge; sorry.

But that was due to the NSM now having checks on the `low' level, which
I think is a mistake.  So I've moved the checks back to the `medium'
level, leaving `low' as a "don't check much of anything" level.

So now the test suite runs as expected again.

I don't have access to a Fedora system to test on, though.

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

Re: The netsec thread

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@mouse.gnus.org>
> Date: Fri, 23 Aug 2019 04:58:35 +0200
> Cc: emacs-devel@gnu.org
> 
> I'm not quite sure what to write in the NEWS file beyond what's already
> there (that the security has been tightened, but that was already the
> case last year :-/), and now it's just slightly more tightened.

If there are any specific changes in behavior due to this
"tightening", we should consider them for NEWS.

Re: The netsec thread

[Robert Pluim]

>>>>> On Fri, 23 Aug 2019 12:09:03 +0300, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Lars Ingebrigtsen <larsi@mouse.gnus.org>
    >> Date: Fri, 23 Aug 2019 04:58:35 +0200
    >> Cc: emacs-devel@gnu.org
    >> 
    >> I'm not quite sure what to write in the NEWS file beyond what's already
    >> there (that the security has been tightened, but that was already the
    >> case last year :-/), and now it's just slightly more tightened.

    Eli> If there are any specific changes in behavior due to this
    Eli> "tightening", we should consider them for NEWS.

I think those were noted in the changes to NEWS brought in from the
branch. I *could* add something like this as well, although I really
am not in favour of advertising the feature too much, hence the
shouting:

+++
** New user option 'nsm-trust-local-network'.
Allows skipping Network Security Manager checks for hosts on your
local subnet(s).  DEFAULTS TO NIL.  There is a good reason for that,
ensure you know what you are doing before you change it.

Re: The netsec thread

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Lars Ingebrigtsen <larsi@mouse.gnus.org>
>> Date: Fri, 23 Aug 2019 04:58:35 +0200
>> Cc: emacs-devel@gnu.org
>> 
>> I'm not quite sure what to write in the NEWS file beyond what's already
>> there (that the security has been tightened, but that was already the
>> case last year :-/), and now it's just slightly more tightened.
>
> If there are any specific changes in behavior due to this
> "tightening", we should consider them for NEWS.

Well...  As technology is found to be insecure, it's added to the list
of what the NSM warns about.

But I don't really think that this is something that needs to be called
out (beyond a general "things are tighter") in either NEWS or documented
in the Emacs manual.

Today, we have stuff like:

@item @acronym{RC4} stream cipher
The @acronym{RC4} stream cipher is believed to be of low quality and
may allow eavesdropping by third parties.  (This is the @code{rc4}
check in @code{network-security-protocol-checks}).

I think this is of interest of absolutely zero reading the Emacs manual,
and is basically security showoffery.  (That's a word.)  The user just
needs to know that we're doing a best-effort er effort to adhere to best
practices, and if they're really really interested, they can read the
doc string to, say, `nsm-protocol-check--dhe-prime-kx', or any of the
other `nsm-protocol-check--*' functions, each of which has an essay in
the doc string now.

So I'd like to propose to remove most of the text about the specific
tests in the "Network Security" node in the Emacs manual (saving
precious pages) and just refer the user to the doc strings.

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

Re: The netsec thread

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: Lars Ingebrigtsen <larsi@mouse.gnus.org>,  emacs-devel@gnu.org
> Date: Fri, 23 Aug 2019 11:40:44 +0200
> 
>     Eli> If there are any specific changes in behavior due to this
>     Eli> "tightening", we should consider them for NEWS.
> 
> I think those were noted in the changes to NEWS brought in from the
> branch. I *could* add something like this as well, although I really
> am not in favour of advertising the feature too much, hence the
> shouting:
> 
> +++
> ** New user option 'nsm-trust-local-network'.
> Allows skipping Network Security Manager checks for hosts on your
> local subnet(s).  DEFAULTS TO NIL.  There is a good reason for that,
> ensure you know what you are doing before you change it.

I don't believe in "security by obscurity", so I think this should be
in NEWS.  Especially since it's in the manual.  I would suggest a
slight rephrase, though:

  +++
  ** New user option 'nsm-trust-local-network'.
  Allows skipping Network Security Manager checks for hosts on your
  local subnet(s).  It defaults to nil.  Usually, there should be no
  need to set this non-nil, and doing that risks opening your local
  network connections to attacks.  So be sure you know what you are
  doing before changing the value.

Thanks.

Re: The netsec thread

[Robert Pluim]

(I removed 'mouse' from Lars' email address)

>>>>> On Fri, 23 Aug 2019 15:18:46 +0300, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Robert Pluim <rpluim@gmail.com>
    >> Cc: Lars Ingebrigtsen <larsi@mouse.gnus.org>,  emacs-devel@gnu.org
    >> Date: Fri, 23 Aug 2019 11:40:44 +0200
    >> 
    Eli> If there are any specific changes in behavior due to this
    Eli> "tightening", we should consider them for NEWS.
    >> 
    >> I think those were noted in the changes to NEWS brought in from the
    >> branch. I *could* add something like this as well, although I really
    >> am not in favour of advertising the feature too much, hence the
    >> shouting:
    >> 
    >> +++
    >> ** New user option 'nsm-trust-local-network'.
    >> Allows skipping Network Security Manager checks for hosts on your
    >> local subnet(s).  DEFAULTS TO NIL.  There is a good reason for that,
    >> ensure you know what you are doing before you change it.

    Eli> I don't believe in "security by obscurity", so I think this should be
    Eli> in NEWS.  Especially since it's in the manual.

I said 'not advertise too much', not 'no documentation' (and itʼs not
in the manual).

    Eli> I would suggest a
    Eli> slight rephrase, though:

    Eli>   +++
    Eli>   ** New user option 'nsm-trust-local-network'.
    Eli>   Allows skipping Network Security Manager checks for hosts on your
    Eli>   local subnet(s).  It defaults to nil.  Usually, there should be no
    Eli>   need to set this non-nil, and doing that risks opening your local
    Eli>   network connections to attacks.  So be sure you know what you are
    Eli>   doing before changing the value.

Sure. Iʼll give it a few days for people to wordsmith it some more.

Robert

Re: The netsec thread

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: rpluim@gmail.com,  emacs-devel@gnu.org
> Date: Fri, 23 Aug 2019 11:58:37 +0200
> 
> @item @acronym{RC4} stream cipher
> The @acronym{RC4} stream cipher is believed to be of low quality and
> may allow eavesdropping by third parties.  (This is the @code{rc4}
> check in @code{network-security-protocol-checks}).
> 
> I think this is of interest of absolutely zero reading the Emacs manual,
> and is basically security showoffery.  (That's a word.)  The user just
> needs to know that we're doing a best-effort er effort to adhere to best
> practices, and if they're really really interested, they can read the
> doc string to, say, `nsm-protocol-check--dhe-prime-kx', or any of the
> other `nsm-protocol-check--*' functions, each of which has an essay in
> the doc string now.
> 
> So I'd like to propose to remove most of the text about the specific
> tests in the "Network Security" node in the Emacs manual (saving
> precious pages) and just refer the user to the doc strings.

I'm firmly against removing existing documentation.  I simply don't
believe it could ever do any harm.

Specifically, regarding these issues, I don't like the paternalistic
attitude "believe us we're doing the best-effort job to adhere to best
practices".  Nothing and no one can assure we know best in every
particular situation, so leaving the knobs for users to DTRT when we
don't cannot be wrong.

I might agree to making the manual descriptions shorter, like
mentioning the variables and pointing to the doc strings for their
detailed descriptions.  But this is only acceptable if the text in the
manual is little more than a copy of the doc string; otherwise we
should enhance the doc strings to tell more.

Thanks.

Re: The netsec thread

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: larsi@gnus.org,  emacs-devel@gnu.org
> Date: Fri, 23 Aug 2019 14:39:05 +0200
> 
> 
> (I removed 'mouse' from Lars' email address)

Lars, please do something about that, relies to your messages bounce.

>     >> +++
>     >> ** New user option 'nsm-trust-local-network'.
>     >> Allows skipping Network Security Manager checks for hosts on your
>     >> local subnet(s).  DEFAULTS TO NIL.  There is a good reason for that,
>     >> ensure you know what you are doing before you change it.
> 
>     Eli> I don't believe in "security by obscurity", so I think this should be
>     Eli> in NEWS.  Especially since it's in the manual.
> 
> I said 'not advertise too much', not 'no documentation' (and itʼs not
> in the manual).

If it is not in the manual, why is it marked "+++"?

>     Eli>   +++
>     Eli>   ** New user option 'nsm-trust-local-network'.
>     Eli>   Allows skipping Network Security Manager checks for hosts on your
>     Eli>   local subnet(s).  It defaults to nil.  Usually, there should be no
>     Eli>   need to set this non-nil, and doing that risks opening your local
>     Eli>   network connections to attacks.  So be sure you know what you are
>     Eli>   doing before changing the value.
> 
> Sure. Iʼll give it a few days for people to wordsmith it some more.

Thanks.

Re: The netsec thread

[Robert Pluim]

>>>>> On Fri, 23 Aug 2019 16:03:33 +0300, Eli Zaretskii <eliz@gnu.org> said:

    Eli> I don't believe in "security by obscurity", so I think this should be
    Eli> in NEWS.  Especially since it's in the manual.
    >> 
    >> I said 'not advertise too much', not 'no documentation' (and itʼs not
    >> in the manual).

    Eli> If it is not in the manual, why is it marked "+++"?

NEWS says:

    Temporary note:
    +++ indicates that all necessary documentation updates are complete.
        (This means all relevant manuals in doc/ AND lisp doc-strings.)

The doc-string is up to date => +++. Itʼs an obscure, hopefully
never-to-be-used option, so I donʼt feel it needs to be in the manual
(you yourself said something similar very recently about
'ispell-skip-html' :-) )

Robert

Re: The netsec thread

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: larsi@gnus.org,  emacs-devel@gnu.org
> Date: Fri, 23 Aug 2019 15:20:47 +0200
> 
> >>>>> On Fri, 23 Aug 2019 16:03:33 +0300, Eli Zaretskii <eliz@gnu.org> said:
> 
>     Eli> If it is not in the manual, why is it marked "+++"?
> 
> NEWS says:
> 
>     Temporary note:
>     +++ indicates that all necessary documentation updates are complete.
>         (This means all relevant manuals in doc/ AND lisp doc-strings.)
> 
> The doc-string is up to date => +++. Itʼs an obscure, hopefully
> never-to-be-used option, so I donʼt feel it needs to be in the manual
> (you yourself said something similar very recently about
> 'ispell-skip-html' :-) )

If this is your interpretation of what NEWS says, then we must modify
that text to be less ambiguous.  "+++" means the doc strings AND the
manuals have been updated, "---" means the doc string WITHOUT manuals
were updated.

These markers are there for when we are close to releasing a new major
version, and need to make sure everything that needs to be in the
manuals is in the manuals.  It therefore makes no sense to mark by
"+++" changes that do not need to be in the manuals, because that
sends the wrong signal.

Re: The netsec thread

[Robert Pluim]

>>>>> On Fri, 23 Aug 2019 17:15:53 +0300, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Robert Pluim <rpluim@gmail.com>
    >> Cc: larsi@gnus.org,  emacs-devel@gnu.org
    >> Date: Fri, 23 Aug 2019 15:20:47 +0200
    >> 
    >> >>>>> On Fri, 23 Aug 2019 16:03:33 +0300, Eli Zaretskii <eliz@gnu.org> said:
    >> 
    Eli> If it is not in the manual, why is it marked "+++"?
    >> 
    >> NEWS says:
    >> 
    >> Temporary note:
    >> +++ indicates that all necessary documentation updates are complete.
    >> (This means all relevant manuals in doc/ AND lisp doc-strings.)
    >> 
    >> The doc-string is up to date => +++. Itʼs an obscure, hopefully
    >> never-to-be-used option, so I donʼt feel it needs to be in the manual
    >> (you yourself said something similar very recently about
    >> 'ispell-skip-html' :-) )

    Eli> If this is your interpretation of what NEWS says, then we must modify
    Eli> that text to be less ambiguous.  "+++" means the doc strings AND the
    Eli> manuals have been updated, "---" means the doc string WITHOUT manuals
    Eli> were updated.

That was indeed my interpretation.

    Eli> These markers are there for when we are close to releasing a new major
    Eli> version, and need to make sure everything that needs to be in the
    Eli> manuals is in the manuals.  It therefore makes no sense to mark by
    Eli> "+++" changes that do not need to be in the manuals, because that
    Eli> sends the wrong signal.

So in this case weʼd use '---'. Itʼs all clear to me now, but youʼre
right that perhaps a different wording would be better (of course it
could just be a problem in my brain :-) )

Robert

Re: The netsec thread

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: larsi@gnus.org,  emacs-devel@gnu.org
> Date: Fri, 23 Aug 2019 16:27:59 +0200
> 
>     Eli> If this is your interpretation of what NEWS says, then we must modify
>     Eli> that text to be less ambiguous.  "+++" means the doc strings AND the
>     Eli> manuals have been updated, "---" means the doc string WITHOUT manuals
>     Eli> were updated.
> 
> That was indeed my interpretation.
> 
>     Eli> These markers are there for when we are close to releasing a new major
>     Eli> version, and need to make sure everything that needs to be in the
>     Eli> manuals is in the manuals.  It therefore makes no sense to mark by
>     Eli> "+++" changes that do not need to be in the manuals, because that
>     Eli> sends the wrong signal.
> 
> So in this case weʼd use '---'.

Yes.

> Itʼs all clear to me now, but youʼre right that perhaps a different
> wording would be better (of course it could just be a problem in my
> brain :-) )

If you can suggest a better wording, please do.  One possible way is
to simply remove the reference to the doc strings, as they are not
relevant to the job these markers should do (doc strings should be
adjusted as part of any change).

Re: The netsec thread

[Robert Pluim]

>>>>> On Fri, 23 Aug 2019 17:40:04 +0300, Eli Zaretskii <eliz@gnu.org> said:

    Eli> If you can suggest a better wording, please do.  One possible way is
    Eli> to simply remove the reference to the doc strings, as they are not
    Eli> relevant to the job these markers should do (doc strings should be
    Eli> adjusted as part of any change).

diff --git a/etc/NEWS b/etc/NEWS
index 3add7a3e6a..1bc01834d0 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -16,10 +16,10 @@ You can narrow news to a specific version by calling 'view-emacs-news'
 with a prefix argument or by typing 'C-u C-h C-n'.
 
 Temporary note:
-+++ indicates that all necessary documentation updates are complete.
-    (This means all relevant manuals in doc/ AND lisp doc-strings.)
++++ indicates that all relevant manuals in doc/ have been updated.
 --- means no change in the manuals is needed.
-When you add a new item, use the appropriate mark if you are sure it applies,
+When you add a new item, use the appropriate mark if you are sure it
+applies, and please also update docstrings as needed.

Re: The netsec thread

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: larsi@gnus.org,  emacs-devel@gnu.org
> Date: Fri, 23 Aug 2019 16:58:12 +0200
> 
>  Temporary note:
> -+++ indicates that all necessary documentation updates are complete.
> -    (This means all relevant manuals in doc/ AND lisp doc-strings.)
> ++++ indicates that all relevant manuals in doc/ have been updated.
>  --- means no change in the manuals is needed.
> -When you add a new item, use the appropriate mark if you are sure it applies,
> +When you add a new item, use the appropriate mark if you are sure it
> +applies, and please also update docstrings as needed.

Fine with me, thanks.

Re: The netsec thread

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 1106 bytes --]

Lars Ingebrigtsen wrote:

> I don't have access to a Fedora system to test on

"make check" works for me on Fedora 30 again; thanks.

> Hm...  Since GnuTLS no longer supports compression at all, I would have
> assumed that on newer OS versions this would be a NOOP.

Yes, the newer GnuTLS implementations are stub functions that are marked 
obsolescent, which causes GCC to warn when you compile code calling them, and if 
--enable-gcc-warnings is used this caused the Emacs build to fail. The fix I 
installed was to call these obsolescent functions only in older GnuTLS versions, 
where they are not obsolescent.

Come to think of it, Emacs shouldn't make the :compression feature visible on 
newer-GnuTLS systems, since the feature is obsolescent and just clutters up the 
runtime and data. So I installed the attached patch, which causes this feature 
(and the :encrypt-then-mac feature) to be exported to the Lisp level only if the 
underlying GnuTLS library supports the feature. This gives a bit more info to 
the Lisp code (if it wants it) and simplifies the data and the low-level code 
slightly.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Tweak-gnutls-peer-status-reporting.patch --]
[-- Type: text/x-patch; name="0001-Tweak-gnutls-peer-status-reporting.patch", Size: 2565 bytes --]

From 80376945952943888bb34c7d4ea06972e422eca7 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 23 Aug 2019 11:50:40 -0700
Subject: [PATCH] Tweak gnutls-peer-status reporting
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/gnutls.c (Fgnutls_peer_status): Report :compression and
:encrypt-then-mac only if the underlying GnuTLS library has
the corresponding features.  This give the Elisp caller a bit
more information about the peer status.
* lisp/net/nsm.el (nsm-protocol-check--compression):
Don’t worry about compression in newer GnuTLS versions
that do not support compression.
---
 lisp/net/nsm.el |  3 ++-
 src/gnutls.c    | 16 +++++++---------
 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/lisp/net/nsm.el b/lisp/net/nsm.el
index ed700bc9b5..5e8381075b 100644
--- a/lisp/net/nsm.el
+++ b/lisp/net/nsm.el
@@ -692,7 +692,8 @@ nsm-protocol-check--compression
 Use of Transport Layer Security (TLS) and Datagram Transport Layer
 Security (DTLS)\", `https://tools.ietf.org/html/rfc7525'"
   (let ((compression (plist-get status :compression)))
-    (and (string-match "^\\bDEFLATE\\b" compression)
+    (and compression
+	 (string-match "^\\bDEFLATE\\b" compression)
          (format-message
           "compression method (%s) may lead to leakage of sensitive information"
           compression))))
diff --git a/src/gnutls.c b/src/gnutls.c
index 51536b1463..a7ef59ab91 100644
--- a/src/gnutls.c
+++ b/src/gnutls.c
@@ -1493,20 +1493,18 @@ returned as the :certificate entry.  */)
 
   /* Compression name. */
 #ifdef HAVE_GNUTLS_COMPRESSION_GET
-  Lisp_Object compression = build_string (gnutls_compression_get_name
-					  (gnutls_compression_get (state)));
-#else
-  Lisp_Object compression = build_string ("NULL");
+  result = nconc2
+    (result, list2 (intern (":compression"),
+		    build_string (gnutls_compression_get_name
+				  (gnutls_compression_get (state)))));
 #endif
-  result = nconc2 (result, list2 (intern (":compression"), compression));
 
   /* Encrypt-then-MAC. */
-  Lisp_Object etm_status = Qnil;
 #ifdef HAVE_GNUTLS_ETM_STATUS
-  if (gnutls_session_etm_status (state))
-    etm_status = Qt;
+  result = nconc2
+    (result, list2 (intern (":encrypt-then-mac"),
+		    gnutls_session_etm_status (state) ? Qt : Qnil));
 #endif
-  result = nconc2 (result, list2 (intern (":encrypt-then-mac"), etm_status));
 
   /* Renegotiation Indication */
   result = nconc2
-- 
2.17.1

Re: The netsec thread

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I'm firmly against removing existing documentation.  I simply don't
> believe it could ever do any harm.
>
> Specifically, regarding these issues, I don't like the paternalistic
> attitude "believe us we're doing the best-effort job to adhere to best
> practices".  Nothing and no one can assure we know best in every
> particular situation, so leaving the knobs for users to DTRT when we
> don't cannot be wrong.

I don't think it's a paternalistic attitude.  I just think these issues
aren't any more interesting to somebody reading the Emacs manual than any
other protocol issue.

In the Emacs manual, we don't discuss details of the SMTP or IMAP
protocols, and we don't give an exegesis on UTF-8.  But for TLS
connections, we currently have a lot of talk about RC4 and 3DES and the
like, and removing that isn't any more a matter of "trust us, we know
what we're doing" than not explaining in detail the difference between
EHLO and HELO in the SMTP protocol.

> I might agree to making the manual descriptions shorter, like
> mentioning the variables and pointing to the doc strings for their
> detailed descriptions.  But this is only acceptable if the text in the
> manual is little more than a copy of the doc string; otherwise we
> should enhance the doc strings to tell more.

The doc strings in the NSM are more detailed than anybody could wish
for, so pointing the Emacs manual readers to those is a good idea, I
think.  Here's the doc string for one of the about 20 checks:

(defun nsm-protocol-check--rsa-kx (host port status &optional settings)
  "Check for static RSA key exchange.

Static RSA key exchange methods do not offer perfect forward
secrecy, therefore, the security of a TLS session is only as
secure as the server's private key.  Due to TLS' use of RSA key
exchange to create a session key (the key negotiated between the
client and the server to encrypt traffic), if the server's
private key had been compromised, the attacker will be able to
decrypt any past TLS session recorded, as opposed to just one TLS
session if the key exchange was conducted via a key exchange
method that offers perfect forward secrecy, such as ephemeral
Diffie-Hellman key exchange.

By default, this check is only enabled when
`network-security-level' is set to `high' for compatibility
reasons.

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'"


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

Re: The netsec thread

[Lars Ingebrigtsen]

Paul Eggert <eggert@cs.ucla.edu> writes:

> Come to think of it, Emacs shouldn't make the :compression feature
> visible on newer-GnuTLS systems, since the feature is obsolescent and
> just clutters up the runtime and data. So I installed the attached
> patch, which causes this feature (and the :encrypt-then-mac feature)
> to be exported to the Lisp level only if the underlying GnuTLS library
> supports the feature. This gives a bit more info to the Lisp code (if
> it wants it) and simplifies the data and the low-level code slightly.

Thanks; makes sense.

I should set up a VM here with Fedora on it to test these things.  I
currently only have Debian and Ubuntu, but those are very similar, so I
don't get much coverage.

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

Re: The netsec thread

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > In the Emacs manual, we don't discuss details of the SMTP or IMAP
  > protocols, and we don't give an exegesis on UTF-8.  But for TLS
  > connections, we currently have a lot of talk about RC4 and 3DES and the
  > like,

Another option is to move that to a comment in the code.
That would not _remove_ it, since it would still be present in Emacs,
but it would not clutter up the manual.

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: The netsec thread

[Lars Ingebrigtsen]

Richard Stallman <rms@gnu.org> writes:

> Another option is to move that to a comment in the code.
> That would not _remove_ it, since it would still be present in Emacs,
> but it would not clutter up the manual.

It's already in the code (as doc strings), so removing it from the
manual is everything that's needed.

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

Re: The netsec thread

[Robert Pluim]

>>>>> On Sun, 25 Aug 2019 07:33:41 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:

    Lars> Paul Eggert <eggert@cs.ucla.edu> writes:
    >> Come to think of it, Emacs shouldn't make the :compression feature
    >> visible on newer-GnuTLS systems, since the feature is obsolescent and
    >> just clutters up the runtime and data. So I installed the attached
    >> patch, which causes this feature (and the :encrypt-then-mac feature)
    >> to be exported to the Lisp level only if the underlying GnuTLS library
    >> supports the feature. This gives a bit more info to the Lisp code (if
    >> it wants it) and simplifies the data and the low-level code slightly.

In a similar vein: TLS1.3 removed support for renegotiation, so should
the following in gnutls-peer-status be made conditional on TLS1.3
having not been negotiated?

  /* Renegotiation Indication */
  result = nconc2
    (result, list2 (intern (":safe-renegotiation"),
                    gnutls_safe_renegotiation_status (state) ? Qt : Qnil));

(And nsm-protocol-check--renegotiation-info-ext probably needs
adjustment as well. I have a patch in progress for that).

Robert

Re: The netsec thread

[Paul Eggert]

Robert Pluim wrote:
> In a similar vein: TLS1.3 removed support for renegotiation, so should
> the following in gnutls-peer-status be made conditional on TLS1.3
> having not been negotiated?
> 
>    /* Renegotiation Indication */
>    result = nconc2
>      (result, list2 (intern (":safe-renegotiation"),
>                      gnutls_safe_renegotiation_status (state) ? Qt : Qnil));

If the Lisp code doesn't care or need it and if it's easy to suppress, it'd make 
sense to do that, yes. I don't have an opinion since I don't know GnuTLS that well.

Re: The netsec thread

[Robert Pluim]

>>>>> On Tue, 3 Sep 2019 06:30:25 -0700, Paul Eggert <eggert@cs.ucla.edu> said:

    Paul> Robert Pluim wrote:
    >> In a similar vein: TLS1.3 removed support for renegotiation, so should
    >> the following in gnutls-peer-status be made conditional on TLS1.3
    >> having not been negotiated?
    >> /* Renegotiation Indication */
    >> result = nconc2
    >> (result, list2 (intern (":safe-renegotiation"),
    >> gnutls_safe_renegotiation_status (state) ? Qt : Qnil));

    Paul> If the Lisp code doesn't care or need it and if it's easy to suppress,
    Paul> it'd make sense to do that, yes. I don't have an opinion since I don't
    Paul> know GnuTLS that well.

The only code that cares is NSM, which can be fixed, and itʼs easy
enough to remove as well. The GNUTLS_TLS1_3 define was added in GnuTLS
3.6.3, so we can check for the version if you prefer.

Robert

diff --git a/src/gnutls.c b/src/gnutls.c
index 042f43e291..9336f1e382 100644
--- a/src/gnutls.c
+++ b/src/gnutls.c
@@ -1519,10 +1519,13 @@ DEFUN ("gnutls-peer-status", Fgnutls_peer_status, Sgnutls_peer_status, 1, 1, 0,
 		    gnutls_session_etm_status (state) ? Qt : Qnil));
 #endif
 
-  /* Renegotiation Indication */
-  result = nconc2
-    (result, list2 (intern (":safe-renegotiation"),
-                    gnutls_safe_renegotiation_status (state) ? Qt : Qnil));
+  /* Renegotiation Indication.  Only for TLS protocol version < 1.3.   */
+#ifdef GNUTLS_TLS1_3
+  if (gnutls_protocol_get_version (state) < GNUTLS_TLS1_3)
+#endif
+    result = nconc2
+      (result, list2 (intern (":safe-renegotiation"),
+                      gnutls_safe_renegotiation_status (state) ? Qt : Qnil));
 
   return result;
 }

Re: The netsec thread

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 603 bytes --]

Robert Pluim wrote:
> The only code that cares is NSM, which can be fixed, and itʼs easy
> enough to remove as well. The GNUTLS_TLS1_3 define was added in GnuTLS
> 3.6.3, so we can check for the version if you prefer.

Checking for GNUTLS_TLS1_3 sounds fine (in fact, a bit better). We can make the 
code a bit faster/clearer by not calling gnutls_protocol_get_version twice. 
Also, it's better to not intertwine ifdefs with ifs. So, something like the 
attached patch perhaps? Though I didn't install it because NSM needs to be 
changed too and I'm not sure what you were thinking of there.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Don-t-mention-safe-renegotiation-in-TLS-1.3.patch --]
[-- Type: text/x-patch; name="0001-Don-t-mention-safe-renegotiation-in-TLS-1.3.patch", Size: 1849 bytes --]

From 0087fd988b03262e1adc04a225e18d2080327515 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 3 Sep 2019 12:16:21 -0700
Subject: [PATCH] =?UTF-8?q?Don=E2=80=99t=20mention=20:safe-renegotiation?=
 =?UTF-8?q?=20in=20TLS=201.3?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/gnutls.c (Fgnutls_peer_status): Don’t put the
safe-renegotiation indication into the status in TLS 1.3, which
removed support for renegotiation.
---
 src/gnutls.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/src/gnutls.c b/src/gnutls.c
index 042f43e291..c74936c840 100644
--- a/src/gnutls.c
+++ b/src/gnutls.c
@@ -1487,10 +1487,10 @@ DEFUN ("gnutls-peer-status", Fgnutls_peer_status, Sgnutls_peer_status, 1, 1, 0,
 				  (gnutls_kx_get (state)))));
 
   /* Protocol name. */
+  gnutls_protocol_t proto = gnutls_protocol_get_version (state);
   result = nconc2
     (result, list2 (intern (":protocol"),
-		    build_string (gnutls_protocol_get_name
-				  (gnutls_protocol_get_version (state)))));
+		    build_string (gnutls_protocol_get_name (proto))));
 
   /* Cipher name. */
   result = nconc2
@@ -1520,9 +1520,15 @@ DEFUN ("gnutls-peer-status", Fgnutls_peer_status, Sgnutls_peer_status, 1, 1, 0,
 #endif
 
   /* Renegotiation Indication */
-  result = nconc2
-    (result, list2 (intern (":safe-renegotiation"),
-                    gnutls_safe_renegotiation_status (state) ? Qt : Qnil));
+#ifdef GNUTLS_TLS1_3
+  bool older_proto = proto < GNUTLS_TLS1_3;
+#else
+  bool older_proto = true;
+#endif
+  if (older_proto)
+    result = nconc2
+      (result, list2 (intern (":safe-renegotiation"),
+		      gnutls_safe_renegotiation_status (state) ? Qt : Qnil));
 
   return result;
 }
-- 
2.17.1

Re: The netsec thread

[Robert Pluim]

>>>>> On Tue, 3 Sep 2019 12:20:27 -0700, Paul Eggert <eggert@cs.ucla.edu> said:

    Paul> Robert Pluim wrote:
    >> The only code that cares is NSM, which can be fixed, and itʼs easy
    >> enough to remove as well. The GNUTLS_TLS1_3 define was added in GnuTLS
    >> 3.6.3, so we can check for the version if you prefer.

    Paul> Checking for GNUTLS_TLS1_3 sounds fine (in fact, a bit better). We can
    Paul> make the code a bit faster/clearer by not calling
    Paul> gnutls_protocol_get_version twice. Also, it's better to not intertwine
    Paul> ifdefs with ifs. So, something like the attached patch perhaps? Though
    Paul> I didn't install it because NSM needs to be changed too and I'm not
    Paul> sure what you were thinking of there.

Below is one way to do it, which would be independent of your
change. Another way would be to check

(plist-member status :safe-renegotiation)

before checking the value of :safe-renegotiation, but that would
require removing :safe-renegotiation for TLS1.3.

I donʼt really prefer one to the other, but the below commit is ready,
and Iʼm lazy :-)

I hope Eli has an opinion, his taste is much better than mine.

Robert

commit d476ad2fcdf6a05cb6f32372df88b1e528682f9e
Author:     Robert Pluim <rpluim@gmail.com>
AuthorDate: Mon Sep 2 14:55:00 2019 +0200
Commit:     Robert Pluim <rpluim@gmail.com>
CommitDate: Tue Sep 3 16:43:12 2019 +0200

    Don't check for safe-renegotiation with TLS1.3
    
    * lisp/net/nsm.el (nsm-protocol-check--renegotiation-info-ext): Don't
    check when using TLS1.3, renegotiation has been removed from TLS.
    Reported in
    <https://lists.gnu.org/archive/html/help-gnu-emacs/2019-09/msg00005.html>

diff --git a/lisp/net/nsm.el b/lisp/net/nsm.el
index 5e8381075b..4515867db8 100644
--- a/lisp/net/nsm.el
+++ b/lisp/net/nsm.el
@@ -665,17 +665,19 @@ nsm-protocol-check--renegotiation-info-ext
 If this TLS extension is not used, the connection established is
 vulnerable to an attack in which an impersonator can extract
 sensitive information such as HTTP session ID cookies or login
-passwords.
+passwords.  Renegotiation was removed in TLS1.3, so this is only
+checked for earlier protocol versions.
 
 Reference:
 
 E. Rescorla, M. Ray, S. Dispensa, N. Oskov (Feb 2010).  \"Transport
 Layer Security (TLS) Renegotiation Indication Extension\",
 `https://tools.ietf.org/html/rfc5746'"
-  (let ((unsafe-renegotiation (not (plist-get status :safe-renegotiation))))
-    (and unsafe-renegotiation
-         (format-message
-          "safe renegotiation is not supported, connection not protected from impersonators"))))
+  (when (string-version-lessp (plist-get status :protocol) "TLS1.3")
+    (let ((unsafe-renegotiation (not (plist-get status :safe-renegotiation))))
+      (and unsafe-renegotiation
+           (format-message
+            "safe renegotiation is not supported, connection not protected from impersonators")))))
 
 ;; Compression checks
 

Re: The netsec thread

[Lars Ingebrigtsen]

Paul Eggert <eggert@cs.ucla.edu> writes:

> * src/gnutls.c (Fgnutls_peer_status): Don’t put the
> safe-renegotiation indication into the status in TLS 1.3, which
> removed support for renegotiation.

[...]

> +#ifdef GNUTLS_TLS1_3
> +  bool older_proto = proto < GNUTLS_TLS1_3;
> +#else
> +  bool older_proto = true;
> +#endif
> +  if (older_proto)
> +    result = nconc2
> +      (result, list2 (intern (":safe-renegotiation"),
> +		      gnutls_safe_renegotiation_status (state) ? Qt : Qnil));

Looks good to me.

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

Re: The netsec thread

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

> -  (let ((unsafe-renegotiation (not (plist-get status :safe-renegotiation))))
> -    (and unsafe-renegotiation
> -         (format-message
> -          "safe renegotiation is not supported, connection not protected from impersonators"))))
> +  (when (string-version-lessp (plist-get status :protocol) "TLS1.3")
> +    (let ((unsafe-renegotiation (not (plist-get status :safe-renegotiation))))
> +      (and unsafe-renegotiation
> +           (format-message
> +            "safe renegotiation is not supported, connection not protected from impersonators")))))

After Paul's patch, I think it would make sense to just check for
whether :safe-renegotiation is present at all, and not check for
protocol version here.  I.e., use `plist-member' first.

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

Re: The netsec thread

[Robert Pluim]

>>>>> On Wed, 04 Sep 2019 15:12:14 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:

    Lars> Robert Pluim <rpluim@gmail.com> writes:
    >> -  (let ((unsafe-renegotiation (not (plist-get status :safe-renegotiation))))
    >> -    (and unsafe-renegotiation
    >> -         (format-message
    >> -          "safe renegotiation is not supported, connection not protected from impersonators"))))
    >> +  (when (string-version-lessp (plist-get status :protocol) "TLS1.3")
    >> +    (let ((unsafe-renegotiation (not (plist-get status :safe-renegotiation))))
    >> +      (and unsafe-renegotiation
    >> +           (format-message
    >> +            "safe renegotiation is not supported, connection not protected from impersonators")))))

    Lars> After Paul's patch, I think it would make sense to just check for
    Lars> whether :safe-renegotiation is present at all, and not check for
    Lars> protocol version here.  I.e., use `plist-member' first.

Sure. Paul, did you want me to push your patch as well?

Robert

Re: The netsec thread

[Paul Eggert]

Robert Pluim wrote:
> Sure. Paul, did you want me to push your patch as well?

Please do.

Re: The netsec thread

[Robert Pluim]

>>>>> On Wed, 4 Sep 2019 14:35:01 -0700, Paul Eggert <eggert@cs.ucla.edu> said:

    Paul> Robert Pluim wrote:
    >> Sure. Paul, did you want me to push your patch as well?

    Paul> Please do.

Done.

Thanks

Robert

Re: The netsec thread

[Robert Pluim]

*sigh* there are days when I hate the C language.

I screwed up, GNUTLS_TLS1_3 is not a define, itʼs an enum, so we canʼt
check for it with the pre-processor. I guess that means we have to
check based on the GnuTLS version, i.e.

diff --git a/src/gnutls.c b/src/gnutls.c
index c74936c840..ffd8e2df4f 100644
--- a/src/gnutls.c
+++ b/src/gnutls.c
@@ -1520,7 +1520,7 @@ DEFUN ("gnutls-peer-status", Fgnutls_peer_status, Sgnutls_peer_status, 1, 1, 0,
 #endif
 
   /* Renegotiation Indication */
-#ifdef GNUTLS_TLS1_3
+#if GNUTLS_VERSION_NUMBER >= 0x030603
   bool older_proto = proto < GNUTLS_TLS1_3;
 #else
   bool older_proto = true;

Re: The netsec thread

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 714 bytes --]

On 9/5/19 5:12 AM, Robert Pluim wrote:
> GNUTLS_TLS1_3 is not a define, itʼs an enum, so we canʼt
> check for it with the pre-processor. I guess that means we have to
> check based on the GnuTLS version

There's a simpler way; I installed the attached.

By the way, can you verify that :safe-renegotiation is also irrelevant 
for DTLS? I'm asking because GNUTLS_DTLS1_2 etc. are greater than 
GNUTLS_TLS1_3 and so "proto <= GNUTLS_TLS1_2" yields 0 for them. I 
assume that since DTLS is for datagrams there is no renegotiation and so 
no :safe-renegotation is needed, but I don't know DTLS (I don't even 
know whether Emacs supports DTLS) and it'd be helpful to get a 
more-expert opinion. Thanks.

[-- Attachment #2: 0001-Port-safe-renegotiation-test-to-GnuTLS-3.6.3.patch --]
[-- Type: text/x-patch, Size: 1120 bytes --]

From 457aee407a6ac0e1ee4c12b9ba919282cce246a3 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 5 Sep 2019 11:42:56 -0700
Subject: [PATCH] Port :safe-renegotiation test to GnuTLS < 3.6.3

Problem reported by Robert Pluim in
https://lists.gnu.org/r/emacs-devel/2019-09/msg00127.html
* src/gnutls.c (Fgnutls_peer_status): Simplify test for
whether the :safe-renegotiation result is needed, so that it
works all the way back to GnuTLS 2.12.2.
---
 src/gnutls.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/src/gnutls.c b/src/gnutls.c
index c74936c840..d43534b5ae 100644
--- a/src/gnutls.c
+++ b/src/gnutls.c
@@ -1520,12 +1520,7 @@ DEFUN ("gnutls-peer-status", Fgnutls_peer_status, Sgnutls_peer_status, 1, 1, 0,
 #endif
 
   /* Renegotiation Indication */
-#ifdef GNUTLS_TLS1_3
-  bool older_proto = proto < GNUTLS_TLS1_3;
-#else
-  bool older_proto = true;
-#endif
-  if (older_proto)
+  if (proto <= GNUTLS_TLS1_2)
     result = nconc2
       (result, list2 (intern (":safe-renegotiation"),
 		      gnutls_safe_renegotiation_status (state) ? Qt : Qnil));
-- 
2.21.0

Re: The netsec thread

[Robert Pluim]

>>>>> On Thu, 5 Sep 2019 11:50:17 -0700, Paul Eggert <eggert@cs.ucla.edu> said:

    Paul> On 9/5/19 5:12 AM, Robert Pluim wrote:
    >> GNUTLS_TLS1_3 is not a define, itʼs an enum, so we canʼt
    >> check for it with the pre-processor. I guess that means we have to
    >> check based on the GnuTLS version

    Paul> There's a simpler way; I installed the attached.

Thanks for that.

    Paul> By the way, can you verify that :safe-renegotiation is also irrelevant
    Paul> for DTLS? I'm asking because GNUTLS_DTLS1_2 etc. are greater than 
    Paul> GNUTLS_TLS1_3 and so "proto <= GNUTLS_TLS1_2" yields 0 for them. I
    Paul> assume that since DTLS is for datagrams there is no renegotiation and
    Paul> so no :safe-renegotation is needed, but I don't know DTLS (I don't
    Paul> even know whether Emacs supports DTLS) and it'd be helpful to get a 
    Paul> more-expert opinion. Thanks.

DTLS does in theory support renegotiation and the safe renegotiation
extension, but Iʼd be surprised if emacs worked with
DTLS. open-network-stream only opens TCP connections, perhaps
make-network-process could be abused to attempt DTLS, but thereʼs no
guarantee it would work, I think most of the emacs low-level TLS
support assumes TCP.

In short: I donʼt think itʼs a problem.

Robert