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

Providing MinRSABits option to limit RSA key length #325

Closed
wants to merge 3 commits into from

Conversation

beldmit
Copy link

@beldmit beldmit commented Jun 10, 2022

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 beldmit force-pushed the minrsabits branch 2 times, most recently from c89e12c to 270bc1b Compare June 24, 2022 12:38
@beldmit
Copy link
Author

beldmit commented Jun 24, 2022

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

@beldmit
Copy link
Author

beldmit commented Jun 25, 2022

Lack of libcbor/libfido2 seem unrelated.

@djmdjm
Copy link
Contributor

djmdjm commented Jul 1, 2022

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.

djmdjm/openssh-wip#13 has a different approach that explicitly checks the key size at the times they are loaded or received

@beldmit
Copy link
Author

beldmit commented Jul 1, 2022

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
Copy link
Author

beldmit commented Aug 17, 2022

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,

@djmdjm
Copy link
Contributor

djmdjm commented Sep 17, 2022

Just added RequiredRSASize in 1875042, 54b333d and 07d8771

@djmdjm djmdjm closed this Sep 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants