Skip to content

Fix cleartext credential exposure in gNMI PAM auth (GHSA-fr8g-q48c-qpc4) - #779

Open
marvellgit wants to merge 1 commit into
masterfrom
advisory-fix-ghsa-fr8g-q48c-qpc4-v2
Open

marvellgit wants to merge 1 commit into
masterfrom
advisory-fix-ghsa-fr8g-q48c-qpc4-v2

Conversation

@marvellgit

Copy link
Copy Markdown

Replace ssh.InsecureIgnoreHostKey() with sshd host key pinning read from /mnt/host/etc/ssh/. Fail closed if keys cannot be loaded and bound a 5-second connect/handshake timeout to prevent rogue listener hangs.

Why I did it

How I did it

How to verify it

Which release branch to backport (provide reason below if selected)

  • 202305
  • 202311
  • 202405
  • 202411
  • 202505
  • 202511
  • 202605

Tracking issue/work item for backport/cherry-pick request (GitHub issue or Microsoft ADO):
Failure type:

Tested branch

  • master
  • 202305
  • 202311
  • 202405
  • 202411
  • 202505
  • 202511
  • 202605
  • N/A

Test result

Description for the changelog

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

Replace ssh.InsecureIgnoreHostKey() with sshd host key pinning read from
/mnt/host/etc/ssh/. Fail closed if keys cannot be loaded and bound a
5-second connect/handshake timeout to prevent rogue listener hangs.

Signed-off-by: marvellgit <marvellgit@users.noreply.github.com>
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@mssonicbld

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Comment thread gnmi_server/pamAuth.go
// SONiC's host-ssh-keygen.sh generates an RSA key by default; ecdsa/ed25519
// are also accepted in case sshd is configured with them.
var sshHostKeyPaths = []string{
"/mnt/host/etc/ssh/ssh_host_ed25519_key.pub",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The fix is correct, but there are no unit tests for sshdHostKeyCallback or the new UserPwAuth logic. To make them testable without a running sshd, change sshAuthAddr from a const to a var (matching what was already done for sshHostKeyPaths):

// const sshAuthAddr = "127.0.0.1:22"
var sshAuthAddr = "127.0.0.1:22"

Then a pam_auth_test.go can cover all the key paths using an in-process SSH server on a random loopback port:

// sshdHostKeyCallback tests
TestCallback_AcceptsPinnedKey          // happy path
TestCallback_RejectsUnpinnedKey        // key mismatch returns error  
TestCallback_AcceptsAnyPinnedAmongMultiple  // multi-key pinning
TestCallback_SkipsMissingFiles         // partial missing → still ok
TestCallback_FailsClosedWithNoReadableFiles // all missing → error
TestCallback_FailsClosedWithInvalidContent  // bad PEM → error
TestCallback_FailsClosedWithTrailingData    // trailing bytes → error

// UserPwAuth tests (override sshHostKeyPaths + sshAuthAddr)
TestUserPwAuth_FailsClosedWhenNoHostKeys   // no key files → false, non-nil err
TestUserPwAuth_AcceptsValidCredentials     // correct password → true
TestUserPwAuth_RejectsWrongPassword        // wrong password → false
TestUserPwAuth_RejectsRogueListener        // server with different key → false

TestUserPwAuth_RejectsRogueListener is the most valuable: it spins up a local SSH server whose host key does not match the pinned one, asserts that UserPwAuth returns false/error without transmitting credentials — directly exercising the F020 attack scenario end-to-end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants