I think “Auth bypass” is a bit of an exaggeration for this issue.
You need to be able to successfully authenticate to the server (i.e. have a valid SSH key) for this attack to work. (Once you authenticate, you can then spoof having logged in as a different user.)
I don’t think that’s right? AIUI, with a server that’s affected you need three things: 1, the name of the user you authenticate as (if the SSH server is set up to check that; some aren’t); 2, the public key of any user that would be let in; 3, any private key at all.
It’s essentially “auth bypass for improperly implemented servers”. Any PublicKeyHandler which was not stateless is potentially vulnerable. This was mainly because the PublicKeyHandler is used for both “checking if a public key would be alright to use” and “actually authenticating with the corresponding private key”.
This is complicated by the ssh.Permissions object, which is is the recommended way to pass data between the auth handlers and the ServerConn. Unfortunately it only contains map[string]string properties, and does not provide a way to smuggle other values. This often leads to people storing values from the PublicKeyHandler and re-using them later. Under the previous version of the library, when the vulnerability is exploited, this would always be incorrect, as the the handler would be called to check if a key would be valid, but never again to check the key when actually authenticating.
This was due to a cache internal in x/crypto/ssh which stored the Permissions result for a given public key. Note that if you were only using Permissions to return data from the PublicKeyHandler, your server should be fine because the final handler call was only skipped if that key wasn’t seen before. The new behavior makes sure to call the handler one last time when verifying the key as well (if it wasn’t the most recently seen key).
It’s not a full bypass, as you do need access to an SSH private key which is authorized for use on that server (to finish the authentication step), and it would need to be a server which was improperly implemented, but that’s unfortunately many of them. In particularly, this is a worry for shared services which allow multiple users to authenticate - they should check their code to make sure if they’re vulnerable.
We were supposed to use SSH Permission Extensions instead of leveraging ssh.Context since baked into that implementation is the idea that the last call to the PublicKeyCallback is the one that will be used for the SSH session.
We will be making a blog post discussing this in more detail.
This supposed “go ssh package” CVE doesn’t really make sense to me.
The function in question already had a doc comment saying “A call to this function does not guarantee that the key offered is in fact used to authenticate …. use Permissions.Extensions”. If you read the docs or knew how the ssh protocol handles keys, you were already secure… and frankly anyone implementing an ssh server should know that clients can offer multiple public keys in arbitrary order before authenticating.
Surely that means the CVEs should be directed at any such applications that misused the function, right?
We don’t have a CVE against the system function in C, we just have CVEs against all the programs that call it with user input.
If a function is frequently misused then there’s a bug in the API design and/or the documentation. If you make it easy to implement an ssh server then you have to expect that non-experts will use the API, so you have to make it easy to use the API correctly even for people who don’t know gnarly details of ssh. This function’s docstring doesn’t say what are the consequences of getting it wrong, so it doesn’t make the security risk clear. (We know it’s unclear because lots of people misread it!)
Compare for example the man page for system(3) which has a SECURITY CONSIDERATIONS section, so you know it’s serious; says the consequences of getting it wrong are command injection vulnerabilities; and outlines what you need to do to avoid it. (It should perhaps advise the reader to use exec(2) instead.)
I think “Auth bypass” is a bit of an exaggeration for this issue.
You need to be able to successfully authenticate to the server (i.e. have a valid SSH key) for this attack to work. (Once you authenticate, you can then spoof having logged in as a different user.)
I don’t think that’s right? AIUI, with a server that’s affected you need three things: 1, the name of the user you authenticate as (if the SSH server is set up to check that; some aren’t); 2, the public key of any user that would be let in; 3, any private key at all.
It’s essentially “auth bypass for improperly implemented servers”. Any PublicKeyHandler which was not stateless is potentially vulnerable. This was mainly because the PublicKeyHandler is used for both “checking if a public key would be alright to use” and “actually authenticating with the corresponding private key”.
This is complicated by the ssh.Permissions object, which is is the recommended way to pass data between the auth handlers and the ServerConn. Unfortunately it only contains
map[string]stringproperties, and does not provide a way to smuggle other values. This often leads to people storing values from the PublicKeyHandler and re-using them later. Under the previous version of the library, when the vulnerability is exploited, this would always be incorrect, as the the handler would be called to check if a key would be valid, but never again to check the key when actually authenticating.This was due to a cache internal in x/crypto/ssh which stored the Permissions result for a given public key. Note that if you were only using Permissions to return data from the PublicKeyHandler, your server should be fine because the final handler call was only skipped if that key wasn’t seen before. The new behavior makes sure to call the handler one last time when verifying the key as well (if it wasn’t the most recently seen key).
It’s not a full bypass, as you do need access to an SSH private key which is authorized for use on that server (to finish the authentication step), and it would need to be a server which was improperly implemented, but that’s unfortunately many of them. In particularly, this is a worry for shared services which allow multiple users to authenticate - they should check their code to make sure if they’re vulnerable.
Notably Gitea and Forgejo both released patches for this yesterday
We recently patched our SSH apps to address this issue, for anyone curious: https://github.com/picosh/pico/commit/dcfb7ac577a343978158c7c4294351ed29ff1f79
We were supposed to use SSH Permission Extensions instead of leveraging
ssh.Contextsince baked into that implementation is the idea that the last call to the PublicKeyCallback is the one that will be used for the SSH session.We will be making a blog post discussing this in more detail.
People have built a small proof-of-concept program to test if a server is vulnerable here: https://codeberg.org/Gusted/CVE-2024-45337
This is actually super helpful - I’ve been looking for a way to test for a vulnerable server in updating a library.
This supposed “go ssh package” CVE doesn’t really make sense to me.
The function in question already had a doc comment saying “A call to this function does not guarantee that the key offered is in fact used to authenticate …. use Permissions.Extensions”. If you read the docs or knew how the ssh protocol handles keys, you were already secure… and frankly anyone implementing an ssh server should know that clients can offer multiple public keys in arbitrary order before authenticating.
Surely that means the CVEs should be directed at any such applications that misused the function, right?
We don’t have a CVE against the
systemfunction in C, we just have CVEs against all the programs that call it with user input.If a function is frequently misused then there’s a bug in the API design and/or the documentation. If you make it easy to implement an ssh server then you have to expect that non-experts will use the API, so you have to make it easy to use the API correctly even for people who don’t know gnarly details of ssh. This function’s docstring doesn’t say what are the consequences of getting it wrong, so it doesn’t make the security risk clear. (We know it’s unclear because lots of people misread it!)
Compare for example the man page for system(3) which has a SECURITY CONSIDERATIONS section, so you know it’s serious; says the consequences of getting it wrong are command injection vulnerabilities; and outlines what you need to do to avoid it. (It should perhaps advise the reader to use exec(2) instead.)
Correctness and practicality don’t always align