openssh-portable icon indicating copy to clipboard operation
openssh-portable copied to clipboard

Providing MinRSABits option to limit RSA key length

Open beldmit opened this issue 3 years ago • 4 comments

There is a need to increase RSA key requirements to make the installations more secure. Just updating the default compiled-in value isn't an option because it may significantly break legacy systems compatibility. This PR introduces a new configuration option to be managed for security's sake.

beldmit avatar Jun 10 '22 14:06 beldmit

Next CI fix provided, could you please reapprove the workflow?

beldmit avatar Jun 24 '22 12:06 beldmit

Lack of libcbor/libfido2 seem unrelated.

beldmit avatar Jun 25 '22 16:06 beldmit

I don't like the approach of adding a module-global minimum size to ssh-rsa.c partly because we're trying to remove global state, but mostly because it causes enforcement of the size limit to be performed in a situation where there is little context as to the origin of the key and so little possibility of showing the user a useful error message.

https://github.com/djmdjm/openssh-wip/pull/13 has a different approach that explicitly checks the key size at the times they are loaded or received

djmdjm avatar Jul 01 '22 06:07 djmdjm

Well, the low-level approach has the advantage that we don't miss any code path by accident. I agree that providing the context is worth efforts. I also wanted to make the patch as little invasive as possible.

Thank you, I will review your implementation

beldmit avatar Jul 01 '22 08:07 beldmit

Our tests found that your patch is incomplete in case when a particular private key is passed via the command line.

I suggest the following fix for it:

@@ -1762,6 +1762,12 @@ load_identity_file(Identity *id)
                       private = NULL;
                       quit = 1;
               }
+              if (r = sshkey_check_rsa_length(private, options.rsa_min_size) != 0) {
+                      debug_fr(r, "Skipping key %s", id->filename);
+                      sshkey_free(private);
+                      private = NULL;
+                      quit = 1;
+              }
               if (!quit && private != NULL && id->agent_fd == -1 &&
                   !(id->key && id->isprivate))
                       maybe_add_key_to_agent(id->filename, private, comment,

beldmit avatar Aug 17 '22 07:08 beldmit

Just added RequiredRSASize in 1875042c52, 54b333d12e and 07d8771bac

djmdjm avatar Sep 17 '22 10:09 djmdjm