Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sts-3.3: Add the preload key #295

Merged
merged 5 commits into from Jul 17, 2017
Merged

Conversation

attilamolnar
Copy link
Contributor

This is one way to check whether a server wishes to be included in preload lists. Another method, proposed by e on IRC is to request a signature from a valid certificate when submitting servers to a preload list.

@jwheare jwheare mentioned this pull request Jan 5, 2017
5 tasks
jwheare
jwheare approved these changes Jan 6, 2017
Copy link
Member

@jwheare jwheare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. No version bump required, the new key is backwards compatible.

@attilamolnar
Copy link
Contributor Author

Added links to sections the text is talking about.

@attilamolnar
Copy link
Contributor Author

attilamolnar commented Jan 10, 2017

Added paragraph telling clients to ignore the preload key unless they are looking to confirm that the server requests to be included in STS preload lists. We could also mention that this (ignoring) applies to the vast majority of clients.

core/sts-3.3.md Outdated
Servers SHOULD verify whether the hostname provided to them via SNI is a
hostname that is whitelisted for preloading by administrators to determine
whether or not to advertise this key.
If no hostname is available via SNI, this key SHOULD NOT be sent.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does HTTP do it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTTP has the Host: header

@attilamolnar
Copy link
Contributor Author

I'll merge this at the end of the week if there are no objections.

@jwheare
Copy link
Member

jwheare commented Jan 15, 2017

Sorry for the late suggestion, but could you flesh out this part some more?

If no hostname is available via SNI, this key SHOULD NOT be sent

It should be explained to a reader why this recommendation is being made, I don't think it's completely obvious as written. An example of the risk to server operators of not checking the hostname would be good.

This was referenced Jan 16, 2017
@lol768
Copy link
Contributor

lol768 commented Feb 2, 2017

An example of the risk to server operators of not checking the hostname would be good.

I guess the main risk here is that an IRC network has other A record entries (or indeed a wildcard entry that points all subdomains at a certain IP address) that they may not want an STS policy stored persistently for - blindly sending the preload key regardless of the hostname sent in the SNI extension would cause these to be stored, if the client was able to open a secure connection. They'd need to be using a wildcard (or multi-SAN cert) for this situation to be applicable, so it's something of an edge case - but probably worth outlining nonetheless.

@attilamolnar
Copy link
Contributor Author

@jwheare which section do you suggest putting the explanation in?

@jwheare
Copy link
Member

jwheare commented Mar 6, 2017

Just right after it's mentioned for now. We can move it around and finesse it if needed once the main gist is written down.

@jwheare
Copy link
Member

jwheare commented Mar 9, 2017

Thanks a lot, I'll take a closer look at this when I have a chance and suggest some improved wording.

…TS policy as a whole, rather than be limited to the preload key
@jwheare
Copy link
Member

jwheare commented Jul 7, 2017

I split out the SNI recommendation to cover the whole policy, rather than be limited to preload.

@jwheare jwheare removed the Last call label Jul 7, 2017
@attilamolnar
Copy link
Contributor Author

@jwheare Makes sense, LGTM

@jwheare
Copy link
Member

jwheare commented Jul 11, 2017

Thanks, any final comments from anyone else before this is merged?

core/sts-3.3.md Outdated
@@ -205,7 +241,8 @@ discussed in General security considerations.
As further protection against bootstrap MITM vulnerabilities, clients may choose to
include a pre-loaded list of known hosts with STS policies. Such lists should be
compiled on an opt-in basis, by request of IRC network administrators. Hosts should be
verified to be correctly advertising an STS policy before inclusion.
verified to be correctly advertising an STS policy, as well as a
[preload policy](#the-preload-key) before inclusion.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make it explicit that preload implies “by request of IRC network administrators”?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How should this be worded to make it more clear to you?

Copy link
Contributor

@progval progval Jul 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

“by request of IRC network administrators via the preload key

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How's this? 4730fb6

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perfect

@@ -274,7 +311,7 @@ server may be offered at a different domain name, for example a subdomain.

It's possible for an attacker to strip the STS `port` value from an initial connection
established via an insecure connection, before the policy has been cached by the client.
This represents a Bootstrap MITM (man-in-the-middle) vulnerability.
This represents a bootstrap MITM (man-in-the-middle) vulnerability.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"MITM" is explained in this part, but not in line 236. Since 236 is earlier it would make sense to move the (man-in-the-middle) up there or remove it completely.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this bothered me a tiny bit too, but well, this is the part that actually talks about it, line 236 is a reference to this part of the spec.

We could alternatively move this section further up, changing the order to:

  • General Security considerations
  • Client implementation considerations
  • Server implementation considerations

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll punt that over to #301 though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Most of the literature I've seen doesn't capitalise the minor words in the initialism, so I'd expect to see MitM(e.g. see http://ieeexplore.ieee.org/document/7546529/?reload=true)

@lol768
Copy link
Contributor

lol768 commented Jul 13, 2017

LGTM, will be nice to see the preload stuff in the spec

@jwheare jwheare merged commit 96f5ef8 into ircv3:master Jul 17, 2017
jwheare added a commit to jwheare/ircv3-specifications that referenced this pull request Jul 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants