search

cds-hooks / docs

CDS Hooks website & specification
http://cds-hooks.org
Apache License 2.0
167 stars 61 forks source link

JWT alg should be standardized #187

Closed isaacvetter closed 5 years ago

isaacvetter commented 6 years ago

In the Trusting EHRs section of the spec, we define a JWT that the EHR presents to the CDS Service. This JWT is used as a bearer token or authentication and is signed by a public key hosted by the EHR.

It would be nice to standardize the signing algorithm used to a specific set that will be supported by all CDS Services, to assure increased compatibility across EHRs and CDS services.

The jwt.io site does a great job listing which algorithms are supported by which commonly used libraries.

I'm going to propose that all CDS Services support both ES384 and HS256.

Are you working on implementing this JWT stuff in your CDS service? What signing algorithms do you support now? What do you think?

isaacvetter commented 6 years ago

@bkaney made the below excellent point in PR #186, I wanted to carry it over into this thread:

Our current alg example shows HS256. For commonality in implementations, the spec should dictate that CDS Services MUST support an algorithm. We're slightly partial to ES386, but HS256 would also work fine. The important thing is to standardize it.

I am not at all a crypto expert, but it is my understanding that HS256 is a symmetric algorithm with only one (secret) key that is shared between the two parties. ES386 is asymmetric with a public/private key pair.

After reading this: https://auth0.com/blog/critical-vulnerabilities-in-json-web-token-libraries/ there is a catch-22 with having the algorithm be discovered in the JWT header of which is supposed to be signed. I think the upshot is the algorithm should be pre-coordinated out-of-band (or specified, as ES386). Thoughts?

kpshek commented 6 years ago

The main vulnerability that blog post pointed out was the use of the none algorithm and the fact that the open source libraries all treated none as a valid algorithm in their signature verification methods. In other words, the API design of those libraries did not espouse a secure-by-default mentality. The changes most of those libraries ended up making was to require the consumer of the library to explicitly say "I am fine trusting JWTs which have a signature that cannot be verified".

An analogy to this example is self-signed TLS certificates. Anyone can create a self-signed certificate for google.com. If developers are working with a library that by default trusts self-signed certificates, we'd have a vulnerability were anyone can pass themselves off as someone else. As it turns out, all of the libraries around TLS I've seen explicitly require you to enable the trusting of self-signed certificates.

The JWT library developers just hadn't learned that lesson (yet).

The second vulnerability called out by this blog post is also around JWT library API design and developers not abiding by secure coding guidelines with respect to verifying the signature. I think it is important to call out this risk -- either in the specification or an implementation guide that provides detailed guidance for developers with respect to the CDS Hooks security model. Right now we're leveraging rfc7515 which requires the alg header. Instructing implementers to ensure the two alg values (in the JWT header and JWK Set) match would be good.

With all of this being said, we definitely need to beef up our documentation around all of this to ensure that it is clear for all implementers and so that they have an excellent resource to ensure they are not allowing any vulnerabilities into their implementations.

Thanks @isaacvetter for starting this discussion because it is an important one we need to have. I'll put some more thought into all of this and respond back later that directly goes towards your questions posed here around aligning on a single/few signing algorithms.

grahamegrieve commented 6 years ago

"all CDS Services support both ES384 and HS256" - what does it mean for a CDS service to 'support' those?

isaacvetter commented 6 years ago

Hey @grahamegrieve - clearly this needs some more thought and discussion.

I think that support would mean that a CDS service could validate a JWT signed with one of these algorithms.

The system constructing the JWT that's sent on the CDS request has to decide which of nine or so algorithms to use to to sign the JWT. How can we ensure that the two+ systems are using the same algorithm?

grahamegrieve commented 6 years ago

I recognise the problem. I just don't know how I'd support ES384.

isaacvetter commented 6 years ago

Grahame - that's excellent feedback! Which algorithms would be easy for you to support?

grahamegrieve commented 6 years ago

I think - all the HS and RS ones are no brainers. it's the EC primitive that's the problem. The problem is which implementers have access to which libraries

bkaney commented 6 years ago

I think - all the HS and RS ones are no brainers. it's the EC primitive that's the problem. The problem is which implementers have access to which libraries

There is support for EC in node, ruby, go, and java at least. It has become popular these days because of blockchain/bitcoin.

I have been working on a prototype and I feel decoding the JWT before it has been verified to obtain information from the header section (e.g. kid and jku), then using the public key to decode (and raising exception if the alg is NONE, or the signature is invalid) is reasonable.

RS/EC are compatible with JWK (JSON Web Key - https://auth0.com/docs/jwks and https://tools.ietf.org/html/rfc7517) as they are asymmetric. I don't think JWK makes sense with HS, since it is symmetric -- both parties would have to figure out how to share a secret out-of-band.

cfeltner commented 6 years ago

Our EHR currently supports RS256 for this. I like the idea of the specification supporting several algorithms with an RS one being included in that supported list.

nwhatt commented 6 years ago

I want to second that there should not be options for both symmetric and asymmetric algorithms. The preference should be for asymmetric only. It's double the implementation and secret management effort. The best practices that most EHRs use for managing certificates for IHE profiles for example can be carried over as well.

isaacvetter commented 6 years ago

Hey @nwhatt !

I just realized that the above was essentially a cross-post from the security zulip stream on the use of JWT in backend services. I definitely want to make the backend services and CDS Hooks use of JWT as similar as possible.

This totally makes sense, provides very specific recommendations to CDS Hooks and the specific references are really nice!

The biggest flaw in JWT is the flexibility of the alg property. Check out the NVD database and most of them boil down to people doing the validation wrong. https://nvd.nist.gov/vuln/search/results?form_type=Basic&results_type=overview&query=JWT&search_type=all

SMART is doing a good thing by insisting on asymmetric keys (rs256) vs symmetric keys (hs256). If you visit jwt.io the default is always going to be hs256 because it's easier to illustrate. It's easier to protect the secret with rs256, and by standardizing on one method, servers won't need to build two separate secret infrastructures - one for hs256 secrets and one for rs256 public keys.

But, I don't really understand what EHR best practices are for managing IHE certs.

The best practices that most EHRs use for managing certificates for IHE profiles for example can be carried over as well.

Could you elaborate?

Isaac

nwhatt commented 6 years ago

Hey @isaacvetter , I'm justing thinking of the hundreds of Epic Interconnect ™ admins out there who already handle certificates for Care Everywhere (IHE profiles), Surescripts, etc. Having them generate a token for says hs256 and securely share it with a third party is a new workflow with arguably harder security constraints.

This could mitigated slightly by having automatic/programatic app registration where the token never needs to be exchanged manually.

JohnMoehrke commented 6 years ago

Can I get the security geeks engaged with a targeted zulip stream for these security and privacy topics? See https://chat.fhir.org/#narrow/stream/Security.20and.20Privacy

I don't want to spread the small community too thin..

I don't think we should conflate the current Certificate Management used to support IHE-ATNA (Node Authentication using mutual-authenticated-TLS) with a certificate management that might be used to support OAuth credentialed apps. Yes, they would be both using X.509, and both using Certs and CA. But there would be a different 'purpose' in the certificate use, and thus should be a different branch.

I would agree that looking to existing organizations that do this RA/CA today might be a quicker way to provision this. Sequoia and/or DirectTrust are candidates; but they would also need to step up. They do have 90% of what is needed involved.

Is that the thinking? @mwhatt

nwhatt commented 6 years ago

@JohnMoehrke I'm thinking it about mostly from the user experience. x.509 certificates will be similar or identical to what the health system employees setting up connections already know. How they issue certificates, key management, and the EHR UI for doing such things theoretically don't need to change from things they already are doing to today.

Granted ECDSA may be extra work for the entire tool chain, but still it's more familiar flow than a shared secret.

kpshek commented 6 years ago

I agree with @bkaney & @nwhatt that we should avoid using symmetric signing algorithms as I do not see any EHR wanting to share their private key with all CDS Service providers in order to validate the signature of the JWT (thereby also allowing a malicious CDS Service provider to impersonate the EHR).

I would advocate we explicitly disallow any symmetric algorithm -- this would include all HMAC (HS*) algorithms.

Additionally, I would advocate we explicitly disallow the none algorithm.

While researching this topic, I came across the proposed rfc7518, which lays out a standard, dubbed JSON Web Algorithms (JWA), for the cryptographic algorithms used by JSON Web Signatures (JWS). Of note is the section on the various signature algorithms. Despite this being a proposed rfc with no movement since 2015, this is the rfc referenced by all of the various JWT libraries. Relevant to this discussion is section 3 which defines the signing algorithms.

rfc7518 specifies that all conforming implementations MUST support HS256. I disagree with this for our use case per my above comments.

Personally, I'd opt for implementing ES384 as:

With this being said, I am not a cryptologist. I do not think we want to standardize on a single algorithm as when we discover that it is no longer secure (either due to flaws or increases in computing power), we would then need to update the CDS Hooks specification to specify the newer algorithms.

Here's a concrete proposal:

JohnMoehrke commented 6 years ago

Well said Kevin. I would not worry about RSA vs ECS. It is true that ECS is more pretty, but today they have equivalent security strength. If platforms support ECS, then yes it should be prefered. The benefit of ECS is it is easier on low end systems while still being strong.

RFC7518 is the right specification, it is just as formal as many other IETF standards we are using. Don't be worried about the status "Proposed Standard", that is normal in IETF for guidance documents.

I would rather there be a bright line between the security and the cds-hooks specification, as there could be uses where vastly different security models are legitimate; and to support revision of security models when issues or new models appear.

It seems to me the main goal of this security specification within cds-hooks is to try to guarantee that the three parties are on the same page. Right? Thus we can start with a small number of possibilities, but will eventually have many. Hence why OAuth is open, and why TLS negotiates inline...

So, I support your recommendation; I just want to encourage separation of security model from core cds-hooks; so that future security models can evolve without breaking the core cds-hooks.

isaacvetter commented 6 years ago

:telephone_receiver: CDS Working Group Call (6-6-2018)

Meeting notes: http://wiki.hl7.org/index.php?title=File:2018-06-06_CDS_WG_Call_Minutes.docx

We talked over this issue during the CDS Working group call today and cam up with a slightly modified version of @kpshek's above proposal:

We also spent time debating the optionality for both the EHR and the CDS service for this support.

@bvdh suggested the following language:

Our goal is to vote on this issue during the HL7 CDS working group call next week.

jmandel commented 6 years ago

I'm coming late to the discussion and want to make sure I understand the expectation here. Without @bvdh's suggestion, if we just said clients and servers should support "ES384 or RS384"... then it would be possible for the client to generate a JWS the "wrong" one (i.e. pick an algorithm that the server didn't support), and there is no discovery protocol in place to even prevent this.

So in short, if we don't want to pick one single algorithm that must be supported by clients and servers alike, then we need something like @bvdh's suggestion to ensure that servers can handle both of our recommendations.

isaacvetter commented 6 years ago

I'd be in favor of limiting the CDS Hooks recommendation to just ES384. It definitely will make the service's job easier. I do think that we should keep the "or better" idea that @dalibaz has been encouraging in SMART backend services.

@bvdh - can you elaborate on why you though RSA was a good idea?

Isaac

bvdh commented 6 years ago

There is some sensitivity regarding the use of elliptic curve crypto.

I agree with @jmandel that from an interoperability point of view, a single algorithm is preferred. On the other hand, most implementers will use a library to validate the token. Most libraries support both algorithms. A quick scan across sevaeral libraries indicated that support of RS384 is a little bit more common than ES384. So most clients will be able able to validate tokens based on either algorithm. In any case, the EMR can choose what to implement (it generates the token).

Than there is the point that Kevin made. We do not want to update the CDS Hooks specification to cope with the current state of recommended encryption algorithms.

Based on these arguments, I come to the following proposal:

jmandel commented 6 years ago

If we're not going to pick any algorithm as required for the CDS service to support (which seems fair), I might suggest adding a "currently supported algorithms list" to the discovery endpoint. This would be helpful at service installation time and potentially also at runtime. it would support a basic automated negotiation over time.

kpshek commented 6 years ago

@isaacvetter & @bvdh - I'm fine with the proposal you all discussed.

@jmandel - I don't see a need to add a list of support algorithms to the Discovery endpoint given that we have already established that registration/integration is an out-of-band process between the EHR and the CDS Service provider. Additionally, this falls into another requirement of the CDS Service and we also have already established that expressing the data elements (prefetch, FHIR resources, FHIR resource fields/profiles, etc) requirements by the CDS Service in the Discovery endpoint is not something we are addressing today.

kpshek commented 6 years ago

@jmandel - Do you want to log a separate issue regarding the inclusion of what signing algorithm(s) the CDS Service understands?

Note - I think this would be related to issues like #330 and (especially) #71

jmandel commented 6 years ago

Can you clarify what you mean by "inclusion" @kpshek? This current issue seems to be all about which signing algorithms we expect/require.

isaacvetter commented 6 years ago

Josh - by inclusion, he just means inclusion in the content at the discovery endpoint.

Overall, there's a number of requests to enhance the discovery endpoint for the CDS service to better describe / document its capabilities and requirements.

Recommending one or more signing algorithms can logically be separated from a CDS service computably describing its support of specific signing algorithms. The CDS wg just voted affirmatively on Bas's proposal which recommends two algo's and doesn't modify the discovery endpoint.

jmandel commented 6 years ago

Cool -- does that mean we're publishing a recommendation for services to support "ES384 or RS384" (rather than "ES384 and RS384")?

isaacvetter commented 6 years ago

Yes, although, it's not totally clear if the optionality applies to the EHR or the CDS service ...

kpshek commented 6 years ago

@jmandel - By "inclusion" I meant including the algorithms to the Discovery response

kpshek commented 6 years ago

:telephone_receiver: CDS Working Group Vote (6-13-2018)

Meeting notes: http://wiki.hl7.org/index.php?title=File:2018-06-13_CDS_WG_Call_Minutes.docx

@kpshek moved the following disposition, seconded by @bvdh.

We will add the following to the specification (per @bvdh's comment above)

:+1: For: 10 :expressionless: Abstain: 0 :-1: Against: 0

:tada: The motion passed! :tada:

kpshek commented 6 years ago

Yes, although, it's not totally clear if the optionality applies to the EHR or the CDS service ...

The recommendation would be for all implementers (EHRs and CDS Service providers). In reality, CDS Service providers are the ones who will need to support multiple signing algorithms as they must verify signatures from various vendors. EHRs on the other hand will likely always sign their JWTs with a single algorithm.