Skip to content

Conversation

ailin-nemui
Copy link
Contributor

No description provided.

@dequis
Copy link
Member

dequis commented Apr 5, 2020

This extension has been deprecated in favour of encouraging IRC networks to use implicit TLS and deploy Strict Transport Security instead. This is in alignment with the recommendations given in RFC 8314

@ailin-nemui
Copy link
Contributor Author

might still be good to use it rather than nothing? sts is currently not possible to implement until they finish the sts host port spec: ircv3/ircv3-specifications#390

@dequis
Copy link
Member

dequis commented Apr 5, 2020

Ugh that one. It's definitely possible to implement without it on our side, that issue just means freenode won't deploy a sts policy on their server without it.

Also, the blocker there is because of certificate validation of the hostname. If that doesn't affect this code, then that means this never validates hostnames.

@ailin-nemui
Copy link
Contributor Author

of course the verification would fail if the hostname was validated. the users can control this with -tls_verify (in theory)

@dequis
Copy link
Member

dequis commented Apr 5, 2020

So if i'm reading this right, this defaults to starttls for every server that doesn't have use_tls already, and handles the lack of starttls by delaying the connection 8 seconds.

  1. This should detect the presence of starttls by using the tls cap, otherwise that's a pointless delay for servers that don't support starttls.

  2. This should not fallback to plaintext if the server is known to support starttls, that's a downgrade attack.

    If TLS cannot be negotiated, it is a hard error and the client must abort the connection

  3. The knowledge of whether the server has supported starttls in the past should be stored persistently somewhere, in the same way as sts, otherwise it's still vulnerable to downgrade attacks. The spec recommends asking the user for confirmation, but it's also okay to upgrade without asking.

    the client should ask the user whether they want to use TLS for all future connections (or until disabled manually). If the user agrees, all future connections to the server must use TLS via STARTTLS

@ailin-nemui
Copy link
Contributor Author

ailin-nemui commented Apr 5, 2020

Thanks for taking your time to read through this.

in my testing, I found that Afternet offers STARTTLS based on the older spec, without announcing it in CAP LS. A CAP-based STARTTLS would be possible but a bit more work. Note that there is no way to support pre-CAP starttls without best effort and delay, or an explicit request from the user as far as I know.

If we want to store the availability of starttls, then we will have to implement some more code in the SERVER_CONNECT_REC, likely similar in what would need to be done for an eventual sts implementation

@ailin-nemui
Copy link
Contributor Author

we have another related issue which is waiting for the cap ls reply. currently, Irssi will send CAP LS and NICK and USER, thus allowing us to log in.

If I wait for CAP LS in order to send STARTTLS, I will move the 8 second delay for the starttls response to an 8 second delay for the CAP LS response. I don't see any other way, you?

The main concern here would be to avoid delaying connections to IRCnet for no reason, which supports neither STARTTLS nor CAP LS (but does support TLS on port 6667)

@dequis
Copy link
Member

dequis commented Apr 5, 2020

in my testing, I found that Afternet offers STARTTLS based on the older spec, without announcing it in CAP LS. A CAP-based STARTTLS would be possible but a bit more work. Note that there is no way to support pre-CAP starttls without best effort and delay, or an explicit request from the user as far as I know.

Yeah. I would ignore this pre-CAP starttls entirely, it's not worth the hassle

If I wait for CAP LS in order to send STARTTLS, I will move the 8 second delay for the starttls response to an 8 second delay for the CAP LS response. I don't see any other way, you?

If the server isn't known to have STARTTLS, send the usual, and if there's a CAP LS response that happens to contain tls, upgrade.

If the server is known to have STARTTLS, then it should be sent right away as the first thing (anything else is a plaintext leak), and if the server doesn't reply to it, that should kill the connection. That should be the only timeout needed, which I think we already have (the server will also be happy to kick us for being quiet, too)

(but does support TLS on port 6667)

Wait is that a typo or a cursed ssl multiplexer?

@ailin-nemui
Copy link
Contributor Author

The problem with "send the usual" is that usually we send CAP LS / PASS / USER / NICK

but we don't want to send that when we can upgrade to starttls or sts.

but if servers do NOT support CAP LS, they will just stall us waiting for the registration sequence

jess in #ircv3 told me that we may get an error out of servers if we send JOIN 0 as the first command

question is if that is how we want to proceed?

yes some ircnet servers are running ssl multiplex, try /connect -tls eu.irc6.net 6667 :-)

@ailin-nemui
Copy link
Contributor Author

I have implemented this idea now

@ailin-nemui
Copy link
Contributor Author

after some playing with it I think I'm too stupid to properly stuff the left-over read buffer into the SSL context. maybe starttls was a stupid idea to begin with @ahf you wouldn't know how to achieve this feat?

@ailin-nemui
Copy link
Contributor Author

ailin-nemui commented Apr 6, 2020

the attached [wip] commit stores the starttls state in the config iff the user has registered this server. (otherwise until a reconnect)

unfortunately, I cannot get /connect -starttls irc.afternet.org to work and I don't understand why. the openssl s_client works, so I guess it must be a bug in my code

@ailin-nemui
Copy link
Contributor Author

fwiw, today /connect -starttls irc.afternet.org does work

@ailin-nemui ailin-nemui force-pushed the starttls branch 3 times, most recently from 3559154 to 8ad2a0e Compare October 26, 2020 23:21
@ailin-nemui
Copy link
Contributor Author

ailin-nemui commented Jan 31, 2021

  • rename nonostarttls to something less awkward, maybe -nodisallow_starttls
    pointed out by ahf

@ailin-nemui ailin-nemui force-pushed the starttls branch 6 times, most recently from 48a8c01 to 3de3bfb Compare April 7, 2021 20:30
@ailin-nemui
Copy link
Contributor Author

implemented the suggestions by ahf

@ailin-nemui ailin-nemui changed the title use starttls use starttls / enable tls_verify Apr 7, 2021
@ailin-nemui ailin-nemui added auto-merge This PR is scheduled for merge if no further comments are opened and removed needs testing labels Apr 7, 2021
@ailin-nemui
Copy link
Contributor Author

@irssi/developers

@ailin-nemui ailin-nemui merged commit 4432b0b into irssi:master Apr 18, 2021
@ailin-nemui ailin-nemui deleted the starttls branch April 18, 2021 10:01
@ailin-nemui ailin-nemui added this to the 1.3.0 milestone Oct 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge This PR is scheduled for merge if no further comments are opened
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants