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

OpenSSHKey: when writing to agent, ensure comment string is at least one byte #1681

Merged

Conversation

tycho
Copy link
Contributor

@tycho tycho commented Mar 8, 2018

Description

gpg-agent doesn't understand how to handle comment strings that are zero-length. It correctly reads the length of the comment string, but then requests a read of 0 bytes, which stupidly returns an EOF error code:

$ gpg-error 67125247
67125247 = (4, 16383) = (GPG_ERR_SOURCE_GPGAGENT, GPG_ERR_EOF) = (GPG Agent, End of file)

I looked at ensuring the m_comment string is always at least length 1, but that turned out to be a much uglier change.

Motivation and context

I'm not able to use the OpenSSH-provided ssh-agent, because most of my SSH keypairs are actually PGP keys stored on YubiKeys -- and those require gpg-agent to use. For the remainder of my on-disk PEM keys, I can load those into KeePassXC... But only if gpg-agent will accept the keys.

How has this been tested?

I found the issue by trying to add my SSH keypairs to KeePassXC and found that gpg-agent was rejecting all of them. I added a bunch of printf debugging to gpg-agent to pin down why it was rejecting them, and it ended up being the comment string.

By sending a comment string that is not zero bytes long, gpg-agent will happily accept the key. I tested this with two unencrypted RSA keys stored in my KeePassXC database.

Types of changes

  • ✅ Bug fix (non-breaking change which fixes an issue)

Checklist:

  • ✅ I have read the CONTRIBUTING document. [REQUIRED]
  • ✅ My code follows the code style of this project. [REQUIRED]
  • ✅ All new and existing tests passed. [REQUIRED]
  • ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

@hifi
Copy link
Member

hifi commented Mar 8, 2018

The username field of an entry is used as the comment if there is none with the key.

@tycho
Copy link
Contributor Author

tycho commented Mar 8, 2018

And if the username field is blank, as is the case for mine, then it breaks gpg-agent...

Copy link
Member

@hifi hifi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some testing and OpenSSH uses the basename of the key file if no comment exists, we should do the same for consistency.

@tycho tycho force-pushed the openssh-gpg-agent-fix branch from fc1cab2 to 7ce2e1d Compare March 8, 2018 19:07
@tycho
Copy link
Contributor Author

tycho commented Mar 8, 2018

How's this?

@hifi hifi removed the needs work label Mar 8, 2018
@hifi
Copy link
Member

hifi commented Mar 8, 2018

👍

@tycho tycho force-pushed the openssh-gpg-agent-fix branch from 7ce2e1d to 05135d5 Compare March 9, 2018 16:28
…one byte

This unbreaks adding keys to gpg-agent.

Signed-off-by: Steven Noonan <[email protected]>
@tycho tycho force-pushed the openssh-gpg-agent-fix branch from 05135d5 to 6a2f7bf Compare March 9, 2018 16:29
@tycho tycho changed the base branch from develop to release/2.3.2 March 9, 2018 16:30
@droidmonkey droidmonkey added this to the v2.3.2 milestone Mar 11, 2018
@droidmonkey droidmonkey merged commit dc1aead into keepassxreboot:release/2.3.2 Mar 11, 2018
@tycho tycho deleted the openssh-gpg-agent-fix branch March 18, 2018 08:28
droidmonkey added a commit that referenced this pull request May 8, 2018
- Enable high entropy ASLR on Windows [#1747]
- Enhance favicon fetching [#1786]
- Fix crash on Windows due to autotype [#1691]
- Fix dark tray icon changing all icons [#1680]
- Fix --pw-stdin not using getPassword function [#1686]
- Fix placeholders being resolved in notes [#1907]
- Enable auto-type start delay to be configurable [#1908]
- Browser: Fix native messaging reply size [#1719]
- Browser: Increase maximum buffer size [#1720]
- Browser: Enhance usability and functionality [#1810, #1822, #1830, #1884, #1906]
- SSH Agent: Parse aes-256-cbc/ctr keys [#1682]
- SSH Agent: Enhance usability and functionality [#1677, #1679, #1681, #1787]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants