[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #23082 [Core Tor/Tor]: tor_addr_parse is overly permissive
#23082: tor_addr_parse is overly permissive
---------------------------------+----------------------------------
Reporter: dcf | Owner: rl1987
Type: defect | Status: needs_review
Priority: Medium | Milestone: Tor: unspecified
Component: Core Tor/Tor | Version: Tor: unspecified
Severity: Normal | Resolution:
Keywords: 032-unreached, ipv6 | Actual Points:
Parent ID: | Points:
Reviewer: | Sponsor:
---------------------------------+----------------------------------
Comment (by rl1987):
Replying to [comment:15 dcf]:
> Replying to [comment:14 rl1987]:
> > https://github.com/torproject/tor/pull/302
>
> Great, rl1987! If you don't mind review from the peanut gallery:
> {{{#!diff
> - if (src[0] == '[' && src[1])
> + if (src[0] == '[' && src[1] && src[strlen(src)-1] == ']') {
> }}}
> I think the condition `src[1]` is now redundant. It was meant to ensure
that the string contains at least 2 characters, but the newly added
condition also does that.
>
I made some cleanups in ad165f7aef4e2feefd6d4cde5d57dfd4fa6d55f5.
> I note that the condition `brackets_detected != 0` is equivalent to `tmp
!= NULL`. But actually, I don't think I would try to remove the
`brackets_detected` variable, because it expresses clearly what it means.
>
That's correct. However, I'm keeping `tmp` variable here, as we may need
to free the temporary bracketless string that `tor_strndup` allocated if
brackets were detected. Let's have both variables.
> I think the `tor_inet_pton(AF_INET6, ...)` line could use a comment
saying that IPv6 addresses are allowed to be with or without brackets.
>
Well, function documentation for `tor_addr_parse()` mentions that already.
I don't think that additional comment is needed.
> {{{#!diff
> + /* Reject if src has needless trailing ':'. */
> + if (len > 2 && src[len - 1] == ':' && src[len - 2] != ':') {
> + result = -1;
> + } else if (tor_inet_pton(AF_INET6, src, &in6_tmp) > 0) {
> }}}
> I wonder, does this check for a trailing colon belong in `tor_inet_pton`
instead? Or is there a reason for `tor_inet_pton` to accept such inputs?
Maybe for compatibility with `inet_pton`? It's surprising to me that this
check is necessary; I would have assumed (wrongly) that the underlying
address parser would reject it.
That's a good idea, as other code that depends on `tor_inet_pton` will be
able to benefit from that check (such as `string_is_valid_ipv6_address()`
function). So I moved the check in
5438ff3e13a803bf24ba98e11a0b1a6b3d839cc9. I'm not sure if that breaks any
standard compatibility though.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/23082#comment:16>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs