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

Offer rsa-sha2-512 and rsa-sha2-256 algorithms in internal SSH #17281

Merged

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Oct 9, 2021

There is a subtle bug in the SSH library x/crypto/ssh which makes the incorrect
assumption that the public key type is the same as the signature algorithm type.

This means that only ssh-rsa signatures are offered by default.

This PR adds a workaround around this problem.

Fix #17175

Signed-off-by: Andrew Thornton art27@cantab.net

There is a subtle bug in the SSH library x/crypto/ssh which makes the incorrect
assumption that the public key type is the same as the signature algorithm type.

This means that only ssh-rsa signatures are offered by default.

This PR adds a workaround around this problem.

Fix go-gitea#17175

Signed-off-by: Andrew Thornton <art27@cantab.net>
@6543
Copy link
Member

6543 commented Oct 11, 2021

Is that bug reported upstream too?

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 11, 2021
modules/ssh/ssh.go Outdated Show resolved Hide resolved
@6543
Copy link
Member

6543 commented Oct 11, 2021

error log: https://hastebin.nl/CViInAB

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Copy link
Contributor Author

error log: https://hastebin.nl/CViInAB

This works on CI, running tests locally and in simple testing here. Was this on your testing?

@codecov-commenter

This comment has been minimized.

@zeripath
Copy link
Contributor Author

Is that bug reported upstream too?

it is but I've added a comment explaining why it's happened.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 13, 2021
@lunny
Copy link
Member

lunny commented Oct 14, 2021

Could you also send a PR to upstream? Maybe they could also give some reviews.

@zeripath
Copy link
Contributor Author

Could you also send a PR to upstream? Maybe they could also give some reviews.

This PR isn't suitable for upstream as it's a workaround. The problem is a fundamental design issue with the upstream ssh.Signer type and its use in the upstream ssh package.

The issue is that the type ssh.Signer:

type Signer interface {
	// PublicKey returns an associated PublicKey instance.
	PublicKey() PublicKey

	// Sign returns raw signature for the given data. This method
	// will apply the hash specified for the keytype to the data.
	Sign(rand io.Reader, data []byte) (*Signature, error)
}

Doesn't provide a mechanism to say what types of algorithm are supported by this Signer. Even

type AlgorithmSigner interface {
	Signer

	// SignWithAlgorithm is like Signer.Sign, but allows specification of a
	// non-default signing algorithm. See the SigAlgo* constants in this
	// package for signature algorithms supported by this package. Callers may
	// pass an empty string for the algorithm in which case the AlgorithmSigner
	// will use its default algorithm.
	SignWithAlgorithm(rand io.Reader, data []byte, algorithm string) (*Signature, error)
}

doesn't do this and nor is it wired into handshake.

It would actually need to be something like:

type TypedSigner interface {
	Signer

	// Type returns the algorithm type for this signer
	Type() String
}

or:

type MultipleAlgorithmSigner interface {
	AlgorithmSigner

	// Algorithms returns the available algorithms 
	Algorithms() []string

	// SignWithAlgorithm is like Signer.Sign, but allows specification of a
	// non-default signing algorithm. See the SigAlgo* constants in this
	// package for signature algorithms supported by this package. Callers may
	// pass an empty string for the algorithm in which case the AlgorithmSigner
	// will use its default algorithm.
	SignWithAlgorithm(rand io.Reader, data []byte, algorithm string) (*Signature, error)
}

This would then allow backwards compatibility with the current system through casting as the interface and not requiring a v2 of the module.

@6543
Copy link
Member

6543 commented Oct 20, 2021

Ok let me test once again ...

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 20, 2021
@techknowlogick techknowlogick merged commit 35b918f into go-gitea:main Oct 20, 2021
zeripath added a commit to zeripath/gitea that referenced this pull request Oct 20, 2021
…tea#17281)

Backport go-gitea#17281

There is a subtle bug in the SSH library x/crypto/ssh which makes the incorrect
assumption that the public key type is the same as the signature algorithm type.

This means that only ssh-rsa signatures are offered by default.

This PR adds a workaround around this problem.

Fix go-gitea#17175

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath added the backport/done All backports for this PR have been created label Oct 20, 2021
wxiaoguang pushed a commit that referenced this pull request Oct 21, 2021
… (#17376)

Backport #17281

There is a subtle bug in the SSH library x/crypto/ssh which makes the incorrect
assumption that the public key type is the same as the signature algorithm type.

This means that only ssh-rsa signatures are offered by default.

This PR adds a workaround around this problem.

Fix #17175

Signed-off-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: techknowlogick <techknowlogick@gitea.io>
zeripath added a commit to zeripath/gitea that referenced this pull request Oct 21, 2021
* SECURITY
  * Upgrade Bluemonday to v1.0.16 (go-gitea#17372) (go-gitea#17374)
  * Ensure correct SSH permissions check for private and restricted users (go-gitea#17370) (go-gitea#17373)
* BUGFIXES
  * Prevent NPE in CSV diff rendering when column removed (go-gitea#17018) (go-gitea#17377)
  * Offer rsa-sha2-512 and rsa-sha2-256 algorithms in internal SSH (go-gitea#17281) (go-gitea#17376)
  * Don't panic if we fail to parse U2FRegistration data (go-gitea#17304) (go-gitea#17371)
  * Ensure popup text is aligned left (backport for 1.15) (go-gitea#17343)
  * Ensure that git daemon export ok is created for mirrors (go-gitea#17243) (go-gitea#17306)
  * Disable core.protectNTFS (go-gitea#17300) (go-gitea#17302)
  * Use pointer for wrappedConn methods (go-gitea#17295) (go-gitea#17296)
  * AutoRegistration is supposed to be working with disabled registration (backport) (go-gitea#17292)
  * Handle duplicate keys on GPG key ring (go-gitea#17242) (go-gitea#17284)
  * Fix SVG side by side comparison link (go-gitea#17375) (go-gitea#17391)

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath mentioned this pull request Oct 21, 2021
@zeripath zeripath deleted the fix-17175-internal-ssh-server-algorithms branch October 21, 2021 19:37
6543 pushed a commit that referenced this pull request Oct 21, 2021
* SECURITY
  * Upgrade Bluemonday to v1.0.16 (#17372) (#17374)
  * Ensure correct SSH permissions check for private and restricted users (#17370) (#17373)
* BUGFIXES
  * Prevent NPE in CSV diff rendering when column removed (#17018) (#17377)
  * Offer rsa-sha2-512 and rsa-sha2-256 algorithms in internal SSH (#17281) (#17376)
  * Don't panic if we fail to parse U2FRegistration data (#17304) (#17371)
  * Ensure popup text is aligned left (backport for 1.15) (#17343)
  * Ensure that git daemon export ok is created for mirrors (#17243) (#17306)
  * Disable core.protectNTFS (#17300) (#17302)
  * Use pointer for wrappedConn methods (#17295) (#17296)
  * AutoRegistration is supposed to be working with disabled registration (backport) (#17292)
  * Handle duplicate keys on GPG key ring (#17242) (#17284)
  * Fix SVG side by side comparison link (#17375) (#17391)

Signed-off-by: Andrew Thornton <art27@cantab.net>
zeripath added a commit to zeripath/gitea that referenced this pull request Oct 22, 2021
Frontport go-gitea#17392

* SECURITY
  * Upgrade Bluemonday to v1.0.16 (go-gitea#17372) (go-gitea#17374)
  * Ensure correct SSH permissions check for private and restricted users (go-gitea#17370) (go-gitea#17373)
* BUGFIXES
  * Prevent NPE in CSV diff rendering when column removed (go-gitea#17018) (go-gitea#17377)
  * Offer rsa-sha2-512 and rsa-sha2-256 algorithms in internal SSH (go-gitea#17281) (go-gitea#17376)
  * Don't panic if we fail to parse U2FRegistration data (go-gitea#17304) (go-gitea#17371)
  * Ensure popup text is aligned left (backport for 1.15) (go-gitea#17343)
  * Ensure that git daemon export ok is created for mirrors (go-gitea#17243) (go-gitea#17306)
  * Disable core.protectNTFS (go-gitea#17300) (go-gitea#17302)
  * Use pointer for wrappedConn methods (go-gitea#17295) (go-gitea#17296)
  * AutoRegistration is supposed to be working with disabled registration (backport) (go-gitea#17292)
  * Handle duplicate keys on GPG key ring (go-gitea#17242) (go-gitea#17284)
  * Fix SVG side by side comparison link (go-gitea#17375) (go-gitea#17391)

Signed-off-by: Andrew Thornton <art27@cantab.net>
Gusted added a commit to Gusted/gitea that referenced this pull request Mar 15, 2022
6543 pushed a commit that referenced this pull request Mar 16, 2022
* Update golang.org/x/crypto

- Update dependency to include fix for CVE.
- See https://groups.google.com/g/golang-announce/c/-cp44ypCT5s/m/wmegxkLiAQAJ?utm_medium=email&utm_source=footer

* Fix deprecation notice

* Remove workaround

- Introduced in #17281
- Fixed in x/crypto:
- golang/crypto@5d542ad
- & golang/crypto@3147a52

* Update Kex Algorithms

- Use standardized name for curve22519-sha256. golang/crypto@9b07691
- Prefer SHA256 version over SHA1 version. golang/crypto@e4b3678
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
…tea#17281)

* Offer rsa-sha2-512 and rsa-sha2-256 algorithms in internal SSH

There is a subtle bug in the SSH library x/crypto/ssh which makes the incorrect
assumption that the public key type is the same as the signature algorithm type.

This means that only ssh-rsa signatures are offered by default.

This PR adds a workaround around this problem.

Fix go-gitea#17175

Signed-off-by: Andrew Thornton <art27@cantab.net>

* as per review

Signed-off-by: Andrew Thornton <art27@cantab.net>
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
* Update golang.org/x/crypto

- Update dependency to include fix for CVE.
- See https://groups.google.com/g/golang-announce/c/-cp44ypCT5s/m/wmegxkLiAQAJ?utm_medium=email&utm_source=footer

* Fix deprecation notice

* Remove workaround

- Introduced in go-gitea#17281
- Fixed in x/crypto:
- golang/crypto@5d542ad
- & golang/crypto@3147a52

* Update Kex Algorithms

- Use standardized name for curve22519-sha256. golang/crypto@9b07691
- Prefer SHA256 version over SHA1 version. golang/crypto@e4b3678
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

key exhange negotiation failed though client and server share some protocols
7 participants