bug#54624: 29.0.50; textsec and ipv6 addresses

bug#54624: 29.0.50; textsec and ipv6 addresses

[Aleksandr Vityazev]

Hi,

in small networks people sometimes use not domain names, but only ipv6
addresses. If (setq textsec-check t), wich is also the default, all such links
are displayed as suspicious. ipv6 addresses contain colons and sometimes
left/right square brackets. Example:

#+begin_src elisp
(textsec-domain-suspicious-p "21a:34aa:c782:3ad2:1bf8:73f8:141:66e8")
 => "Disallowed character: : (#x3a, COLON)"
(textsec-domain-suspicious-p "[21a:34aa:c782:3ad2:1bf8:73f8:141:66e8]")
 => "Disallowed character: [ (#x5b, LEFT SQUARE BRACKET)"
#+end_src

I think it is not entirely correct. It may be possible to add additional
validation for such addresses in the textsec-domain-suspicious-p function.
WDYT?


-- 
Best regards,
Aleksandr Vityazev

bug#54624: 29.0.50; textsec and ipv6 addresses

[Lars Ingebrigtsen]

Aleksandr Vityazev <avityazev@posteo.org> writes:

> #+begin_src elisp
> (textsec-domain-suspicious-p "21a:34aa:c782:3ad2:1bf8:73f8:141:66e8")
>  => "Disallowed character: : (#x3a, COLON)"
> (textsec-domain-suspicious-p "[21a:34aa:c782:3ad2:1bf8:73f8:141:66e8]")
>  => "Disallowed character: [ (#x5b, LEFT SQUARE BRACKET)"
> #+end_src
>
> I think it is not entirely correct. It may be possible to add additional
> validation for such addresses in the textsec-domain-suspicious-p function.
> WDYT?

Yup; I've now done so in Emacs 29.

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

bug#54624: 29.0.50; textsec and ipv6 addresses

[Mattias Engdegård]

>   (or (string-match-p "\\`\\([0-9]\\{1,3\\}\\.?\\)\\{1,4\\}\\'" domain)
>       (let ((ipv6 "\\([0-9a-f]\\{0,4\\}:?\\)\\{1,8\\}"))

Sorry, but these regexps aren't very precise, are they? They match a lot more than valid IP addresses. For example, both match "12345".

Is there any reason to be approximate here? Shouldn't be very hard to only allow well-formed addresses.

bug#54624: 29.0.50; textsec and ipv6 addresses

[Andreas Schwab]

On Apr 03 2022, Mattias Engdegård wrote:

> Sorry, but these regexps aren't very precise, are they? They match a lot more than valid IP addresses. For example, both match "12345".

12345 is actually a valid IPv4 address, same as 0.0.48.57.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#54624: 29.0.50; textsec and ipv6 addresses

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> Is there any reason to be approximate here? Shouldn't be very hard to
> only allow well-formed addresses.

The point here isn't to validate an address.  It's to say whether
there's something suspicious in an URL.

There's nothing particularly suspicious about an URL that has an invalid
domain part (in itself).

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

bug#54624: 29.0.50; textsec and ipv6 addresses

[Mattias Engdegård]

3 apr. 2022 kl. 18.58 skrev Andreas Schwab <schwab@linux-m68k.org>:

> 12345 is actually a valid IPv4 address, same as 0.0.48.57.

While this is correct, I'm not sure it was Lars's intention to cover addresses in that format, or that it would be desirable to do so in the context of textsec. Do you agree?

bug#54624: 29.0.50; textsec and ipv6 addresses

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> While this is correct, I'm not sure it was Lars's intention to cover
> addresses in that format, or that it would be desirable to do so in
> the context of textsec. Do you agree?

That was indeed my intention.

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

bug#54624: 29.0.50; textsec and ipv6 addresses

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>> While this is correct, I'm not sure it was Lars's intention to cover
>> addresses in that format, or that it would be desirable to do so in
>> the context of textsec. Do you agree?
>
> That was indeed my intention.

(But that's separate from the new ipvx function -- that one was already
not deemed suspicious before that function was added.)

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

bug#54624: 29.0.50; textsec and ipv6 addresses

[Mattias Engdegård]

3 apr. 2022 kl. 19.02 skrev Lars Ingebrigtsen <larsi@gnus.org>:

> The point here isn't to validate an address.  It's to say whether
> there's something suspicious in an URL.
> 
> There's nothing particularly suspicious about an URL that has an invalid
> domain part (in itself).

Fair enough. The reason I'm looking at this is that relint complained about some parts (repeated match of empty string) and indeed the regexps do look a bit inefficient.

Would you mind if I removed some ambiguity from the regexps then? Maybe even simplifying them to "[0-9.]+" and "[0-9a-f:]+" respectively?

bug#54624: 29.0.50; textsec and ipv6 addresses

[Andreas Schwab]

On Apr 03 2022, Mattias Engdegård wrote:

> Would you mind if I removed some ambiguity from the regexps then? Maybe even simplifying them to "[0-9.]+" and "[0-9a-f:]+" respectively?

Note that an IPv6 address notation can contain dots too.  This is useful
for mapped IPv4 addresses.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#54624: 29.0.50; textsec and ipv6 addresses

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> Would you mind if I removed some ambiguity from the regexps then?
> Maybe even simplifying them to "[0-9.]+" and "[0-9a-f:]+"
> respectively?

Fine by me, but addresses that are too long should be suspicious (to
catch people obfuscating by doing things like 000000000000000127.0.0.1
and similar).  That's what I was intending to catch with the {} things
without having to actually do the hard maths myself.  :-)

And as Andreas said, I had forgotten that IPv6 addresses can contain
dots, so if you could fix that at the same time...

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

bug#54624: 29.0.50; textsec and ipv6 addresses

[Mattias Engdegård]

4 apr. 2022 kl. 12.42 skrev Lars Ingebrigtsen <larsi@gnus.org>:

> Fine by me, but addresses that are too long should be suspicious (to
> catch people obfuscating by doing things like 000000000000000127.0.0.1
> and similar).  That's what I was intending to catch with the {} things
> without having to actually do the hard maths myself.  :-)

I agree that is desirable. If a strict parse is impractical (not sure if it is), what about something slightly stricter than what we current have? Here is a straw-man proposal:

    (rx-let ((octet (or "0" (: (in "1-9") (? (in "0-9") (? (in "0-9"))))))
             (ipv4 (: octet (= 3 "."  octet)))
             (hextet (** 1 4 (in "0-9a-f")))
             (ipv6 (: (? "::") hextet (* ":" (? ":") hextet)
                      (? (or "::" (: ":" ipv4) )))))
      (rx bos (or ipv4 ipv6 (: "[" ipv6 "]")) eos))

and don't forget to bind case-fold-search to nil while calling string-match-p since IPv6 specifies lower-case hex digits.

And thanks to Andreas for reminding me about IPv6 allowing dotted quads as well.

bug#54624: 29.0.50; textsec and ipv6 addresses

[Robert Pluim]

>>>>> On Mon, 4 Apr 2022 14:48:37 +0200, Mattias Engdegård <mattiase@acm.org> said:

    Mattias> 4 apr. 2022 kl. 12.42 skrev Lars Ingebrigtsen <larsi@gnus.org>:
    >> Fine by me, but addresses that are too long should be suspicious (to
    >> catch people obfuscating by doing things like 000000000000000127.0.0.1
    >> and similar).  That's what I was intending to catch with the {} things
    >> without having to actually do the hard maths myself.  :-)

    Mattias> I agree that is desirable. If a strict parse is impractical (not sure if it is), what about something slightly stricter than what we current have? Here is a straw-man proposal:

    Mattias>     (rx-let ((octet (or "0" (: (in "1-9") (? (in "0-9") (? (in "0-9"))))))
    Mattias>              (ipv4 (: octet (= 3 "."  octet)))
    Mattias>              (hextet (** 1 4 (in "0-9a-f")))
    Mattias>              (ipv6 (: (? "::") hextet (* ":" (? ":") hextet)
    Mattias>                       (? (or "::" (: ":" ipv4) )))))
    Mattias>       (rx bos (or ipv4 ipv6 (: "[" ipv6 "]")) eos))

    Mattias> and don't forget to bind case-fold-search to nil while calling string-match-p since IPv6 specifies lower-case hex digits.

    Mattias> And thanks to Andreas for reminding me about IPv6 allowing dotted quads as well.

Or we just arrange it so that thereʼs a primitive that maps to calling
getaddrinfo(3) with AI_NUMERICHOST in the hints.ai_flags (but Lars'
motivation here is 'not suspicious', not 'looks like a numeric IP
address', so maybe not).

Robert
-- 

bug#54624: 29.0.50; textsec and ipv6 addresses

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> I agree that is desirable. If a strict parse is impractical (not sure if it is), what about something slightly stricter than what we current have? Here is a straw-man proposal:
>
>     (rx-let ((octet (or "0" (: (in "1-9") (? (in "0-9") (? (in "0-9"))))))
>              (ipv4 (: octet (= 3 "."  octet)))
>              (hextet (** 1 4 (in "0-9a-f")))
>              (ipv6 (: (? "::") hextet (* ":" (? ":") hextet)
>                       (? (or "::" (: ":" ipv4) )))))
>       (rx bos (or ipv4 ipv6 (: "[" ipv6 "]")) eos))

Skimming that, it seems a bit too strict, but perhaps I'm misreading it.

> and don't forget to bind case-fold-search to nil while calling string-match-p since IPv6 specifies lower-case hex digits.

Again, we're not trying to create a strict validator here.  We're just
saying something about suspiciousness -- invalid addresses aren't, in
and of themselves, suspicious.

(And most resolvers will accept upper-case hex digits just fine.)

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

bug#54624: 29.0.50; textsec and ipv6 addresses

[Mattias Engdegård]

6 apr. 2022 kl. 11.19 skrev Lars Ingebrigtsen <larsi@gnus.org>:

> Skimming that, it seems a bit too strict, but perhaps I'm misreading it.

In what way is it too strict? I can't make it less strict unless you tell me.

> Again, we're not trying to create a strict validator here.  We're just
> saying something about suspiciousness -- invalid addresses aren't, in
> and of themselves, suspicious.

That's fine, but "suspicious" is quite subjective and I need your help to define it better if I'm to write a simulator of your suspicion.

Here is a new attempt, simplified a bit:

  (rx-let ((octet (** 1 3 (in "0-9")))
           (ipv4 (: octet (= 3 "."  octet)))
           (hextet (** 1 4 (in "0-9a-f")))
           (ipv6 (: (? "::") hextet (* (or ":" "::") hextet)
                    (? ":" (or ":" ipv4)))))
    (string-match-p (rx bos (or ipv4 ipv6 (: "[" ipv6 "]")) eos)
                    domain)))

We could simplify it further and relax the requirement on octets and hextets (which seems to be what they are called, rather incongruously) but then it wouldn't catch what you previously thought were suspicious.

bug#54624: 29.0.50; textsec and ipv6 addresses

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> Here is a new attempt, simplified a bit:
>
>   (rx-let ((octet (** 1 3 (in "0-9")))
>            (ipv4 (: octet (= 3 "."  octet)))
>            (hextet (** 1 4 (in "0-9a-f")))
>            (ipv6 (: (? "::") hextet (* (or ":" "::") hextet)
>                     (? ":" (or ":" ipv4)))))
>     (string-match-p (rx bos (or ipv4 ipv6 (: "[" ipv6 "]")) eos)
>                     domain)))

I don't really read rx syntax well, but that seems reasonable to me on
first sight.

> We could simplify it further and relax the requirement on octets and
> hextets (which seems to be what they are called, rather incongruously)
> but then it wouldn't catch what you previously thought were
> suspicious.

You misunderstand -- the function wasn't introduced to say something
about (certain) raw addresses being suspicious.  It was introduced to
say that they aren't.

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

bug#54624: 29.0.50; textsec and ipv6 addresses

[Mattias Engdegård]

7 apr. 2022 kl. 13.04 skrev Lars Ingebrigtsen <larsi@gnus.org>:

> I don't really read rx syntax well, but that seems reasonable to me on
> first sight.

Sorry! Here is a more conservative change that only alters a few minor details in your original code, written in the way that you prefer:

  (let* ((ipv4 "\\(?:[0-9]\\{1,3\\}\\.?\\)\\{1,4\\}")
         (ipv6 (concat "\\(?:[0-9a-f]\\{0,4\\}:\\)\\{1,8\\}[0-9a-f]\\{0,4\\}"
                       "\\(?::" ipv4 "\\)?")))
    (string-match-p
     (concat "\\`\\(?:" ipv4 "\\|" ipv6 "\\|\\[" ipv6 "\\]\\)\\'")
     domain))

bug#54624: 29.0.50; textsec and ipv6 addresses

[Andreas Schwab]

On Apr 07 2022, Mattias Engdegård wrote:

> Sorry! Here is a more conservative change that only alters a few minor details in your original code, written in the way that you prefer:
>
>   (let* ((ipv4 "\\(?:[0-9]\\{1,3\\}\\.?\\)\\{1,4\\}")
>          (ipv6 (concat "\\(?:[0-9a-f]\\{0,4\\}:\\)\\{1,8\\}[0-9a-f]\\{0,4\\}"
>                        "\\(?::" ipv4 "\\)?")))
>     (string-match-p
>      (concat "\\`\\(?:" ipv4 "\\|" ipv6 "\\|\\[" ipv6 "\\]\\)\\'")
>      domain))

That won't match "::ffff:93.184.216.34".

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#54624: 29.0.50; textsec and ipv6 addresses

[Mattias Engdegård]

8 apr. 2022 kl. 00.13 skrev Andreas Schwab <schwab@linux-m68k.org>

> That won't match "::ffff:93.184.216.34".

Computer disagrees!

bug#54624: 29.0.50; textsec and ipv6 addresses

[Andreas Schwab]

On Apr 08 2022, Mattias Engdegård wrote:

> 8 apr. 2022 kl. 00.13 skrev Andreas Schwab <schwab@linux-m68k.org>
>
>> That won't match "::ffff:93.184.216.34".
>
> Computer disagrees!

And the computer is always right, of course.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#54624: 29.0.50; textsec and ipv6 addresses

[Mattias Engdegård]

8 apr. 2022 kl. 11.18 skrev Andreas Schwab <schwab@linux-m68k.org>:

>> Computer disagrees!
> 
> And the computer is always right, of course.

Tell me all about it, it's been like this all morning. I've even threatened with reinstallation, to no avail.
No proper respect towards us organics.

bug#54624: 29.0.50; textsec and ipv6 addresses

[Mattias Engdegård]

I ended up with a conservative approach of only changing the minimum necessary (support for IPv6/IPv4 hybrid addresses, remove regexp ambiguity) but did translate to rx. If you prefer otherwise, tell me and It will be done.

Changed on master (26db1ca80e).

bug#54624: 29.0.50; textsec and ipv6 addresses

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> I ended up with a conservative approach of only changing the minimum
> necessary (support for IPv6/IPv4 hybrid addresses, remove regexp
> ambiguity) but did translate to rx. If you prefer otherwise, tell me
> and It will be done.

Looks fine to me.

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