GNOME Bugzilla – Bug 790910
ssh-agent interface does not support SHA2 extension
Last modified: 2018-06-30 17:03:47 UTC
Created attachment 364516 [details] [review] Support SHA2 extension for RSA signatures OpenSSH since version 7.2 (Feb 2016) [0] is using SHA2 extension to provide RSA signatures using safe hash algorithm [1]. This extension is not negotiated with the ssh-agents and so far it worked because of the bug in OpenSSH signature verification [2], but it will hopefully be soon fixed and at that time, RSA signatures provided be gnome-keyring will stop working (because it is ignoring the passed flags). The attached patch adds a support to provide these signatures. The testsuite is adjusted accordingly and passes. Also the behavior was verified against upstream OpenSSH with the patch attached to the OpenSSH bug. [0] http://www.openssh.com/txt/release-7.2 [1] https://tools.ietf.org/html/draft-ietf-curdle-rsa-sha2-12 [2] https://bugzilla.mindrot.org/show_bug.cgi?id=2799
Review of attachment 364516 [details] [review]: Thank you, nice. I only have a stylistic comment about the gkd_ssh_agent_proto_algo_to_keytype function. ::: daemon/ssh-agent/gkd-ssh-agent-proto.c @@ +151,2 @@ const gchar* +gkd_ssh_agent_proto_algo_to_keytype (gulong algo, GQuark curve_oid, GChecksumType halgo) This interface is becoming a bit bloated; curve_oid is only relevant to ECC, and halgo is only relevant to RSA. It is not obvious how this function is supposed to work, by only looking at the caller. I would suggest to either: - expand this at the caller side, or - split this function to: gkd_ssh_agent_proto_rsa_algo_to_keytype(algo, halgo) gkd_ssh_agent_proto_ec_algo_to_keytype(algo, curve_oid) gkd_ssh_agent_proto_dsa_algo_to_keytype(algo) This would increase the code complexity, but I guess it would be acceptable amount, given that there are only two places where the function is called. @@ +682,3 @@ } + salgo = gkd_ssh_agent_proto_algo_to_keytype (algo, oid, 0); Here, 0 == G_CHECKSUM_MD5. As I mentioned above, it's not obvious how gkd_ssh_agent_proto_algo_to_keytype() treats that value without seeing the function definition.
Created attachment 364557 [details] [review] Support SHA2 extension for RSA signatures v2 Thank you for the review. It was a bit cumbersome. I revised it and expanded the problematic parts little bit. And created separate functions for separate key types and moved the handling of them into keytype-specific places. It makes the code little bit longer, but hopefully more understandable. For the gkd_ssh_agent_proto_write_public(), now I used directly SHA1 constant (default), rather than bogus 0, since this is not identification of signature algorithm, but simple raw key type of the same name. I hope it is more clear now.
Review of attachment 364557 [details] [review]: Thank you, looks fine.
(In reply to Jakub Jelen from comment #0) > ... but it will hopefully be soon fixed and > at that time, RSA signatures provided be gnome-keyring will stop working > (because it is ignoring the passed flags). Does that mean we should backport the patch to the stable branch?
I don't think it is needed. Next regular release should be fine, since the change in OpenSSH will not be earlier than in their next release (in 3 to 6 months I guess) and then there will be some time before distributions will update if they ever do.
OK, thanks for the confirmation. Pushed as: https://git.gnome.org/browse/gnome-keyring/commit/?id=35a01f8c6eaf3c991aaeb3f66449f41d3f0580bc