Skip to content

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 [email protected]

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 <[email protected]>
@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
@6543
Copy link
Member

6543 commented Oct 11, 2021

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

@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 <[email protected]>
@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 <[email protected]>

Co-authored-by: techknowlogick <[email protected]>
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 <[email protected]>
@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 <[email protected]>
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 <[email protected]>
Gusted pushed 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 <[email protected]>

* as per review

Signed-off-by: Andrew Thornton <[email protected]>
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