Skip to content

uams: introduced static library with common auth functions#2932

Draft
rdmark wants to merge 9 commits into
mainfrom
rdmark-dhx2-hardening
Draft

uams: introduced static library with common auth functions#2932
rdmark wants to merge 9 commits into
mainfrom
rdmark-dhx2-hardening

Conversation

@rdmark

@rdmark rdmark commented Apr 25, 2026

Copy link
Copy Markdown
Member

DHX2: Zero-pad the DH prime p when serializing for the wire, matching the existing treatment of g and Ma. Also guard logincont2 and changepw_3 against being called before the shared key K is established by logincont1, preventing a NULL pointer dereference if a client skips the key exchange step.

DHX: Guard logincont and changepw against being called before the shared key K is established, preventing a NULL pointer dereference if the login step was skipped or failed. Also release K immediately after serializing it to the local key buffer, fixing a leak that left key material in memory indefinitely after authentication.

SRP: Check that the SRP session verifier is set before proceeding in srp_logincont, preventing a NULL pointer dereference if the login step was skipped or failed.

Inspired by bug fixes in afp-perl by Derrik Pates

@augmentcode

augmentcode Bot commented Apr 25, 2026

Copy link
Copy Markdown
🤖 Augment PR Summary

Summary: This PR hardens the DHX2 UAM key exchange by making wire serialization and state handling more robust.

Changes:

  • Zero-pads the DH prime p during DHX2 setup so it is always serialized to a fixed PRIMEBITS/8 length, matching existing padding behavior for g and Ma.
  • Adds guards in DHX2 logincont2 handlers to reject calls made before logincont1 established the shared key (K_MD5hash), preventing NULL dereferences.
  • Adds a similar guard in the PAM variant’s changepw_3 to ensure the key exchange step completed before decrypting password buffers.

Technical Notes: The changes align wire formatting with Apple’s expectations and harden against clients that skip required exchange steps (inspired by afp-perl fixes).

🤖 Was this summary useful? React with 👍 or 👎

@augmentcode augmentcode Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review completed. 1 suggestion posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

Comment thread etc/uams/uams_dhx2_pam.c Outdated
@rdmark
rdmark force-pushed the rdmark-dhx2-hardening branch from 63758fa to 08a96e4 Compare April 25, 2026 16:24
@rdmark

rdmark commented Apr 25, 2026

Copy link
Copy Markdown
Member Author

the heavy code repetition is a recurring concern with these UAMs -- we've had a few corner case bugs in the past because of this. I have been pondering alternative solutions: such as a static library with abstractions for the repeated logic, or maybe even merge the respective PAM and passwd libraries into one and make the authentication backend a runtime option.

@rdmark
rdmark requested review from andylemin May 6, 2026 17:57
@rdmark rdmark changed the title uams: harden DHX2 key exchange and prime serialization uams: harden DHX2, DHX, SRP key exchange and session verifier May 6, 2026
@andylemin

andylemin commented May 7, 2026

Copy link
Copy Markdown
Contributor

the heavy code repetition is a recurring concern with these UAMs -- we've had a few corner case bugs in the past because of this. I have been pondering alternative solutions: such as a static library with abstractions for the repeated logic, or maybe even merge the respective PAM and passwd libraries into one and make the authentication backend a runtime option.

Yes we should try to have shared code if we can - this is going to be prone to issues as you say, which is not a good thing for the front security layer

rdmark added 7 commits May 14, 2026 20:59
Zero-pad the DH prime p when serializing for the wire, matching
the existing treatment of g and Ma. Also guard logincont2 and
changepw_3 against being called before the shared key K is
established by logincont1, preventing a NULL pointer dereference
if a client skips the key exchange step.

Add error handling for all gcry_mpi_print calls when serializing
g, p, and Ma in dhx2_setup. If serialization fails, bail out
via the existing error path instead of using a stale nwritten
value for the zero-padding logic.

Inspired by bug fixes in afp-perl by Derrik Pates
Guard logincont and changepw against being called before the
shared key K is established, preventing a NULL pointer dereference
if the login step was skipped or failed. Also release K immediately
after serializing it to the local key buffer, fixing a leak that
left key material in memory indefinitely after authentication.

Add error handling for all gcry_mpi_print calls in DHX UAMs.
In the setup functions, bail out via the existing error path if
serialization of K or Mb fails. In logincont and changepw,
release K and return an error instead of using a stale nwritten
value for the zero-padding logic.

Inspired by bug fixes in afp-perl by Derrik Pates
Check that the SRP session verifier is set before proceeding
in srp_logincont, preventing a NULL pointer dereference if the
login step was skipped or failed.
@rdmark
rdmark force-pushed the rdmark-dhx2-hardening branch from 6681a2e to e28f738 Compare May 14, 2026 18:59
@rdmark
rdmark temporarily deployed to github-pages May 14, 2026 19:02 — with GitHub Actions Inactive
@rdmark
rdmark temporarily deployed to github-pages May 14, 2026 19:35 — with GitHub Actions Inactive
@sonarqubecloud

Copy link
Copy Markdown

@rdmark
rdmark temporarily deployed to github-pages May 14, 2026 20:02 — with GitHub Actions Inactive
@github-actions

Copy link
Copy Markdown
Contributor

🔥 Spectest (AFP 3.4) - Flamegraph (AFP_ASSERT active)

Commit: 5c815d4b478ea8b77352c18c73a2a359f314bc86
Profiling: On-CPU sampling @ 1019 Hz (prime), DWARF call-graph, x86_64
Build: debugoptimized (-O2 -g -fno-omit-frame-pointer)
Total Runtime: 67s, Netatalk Code-time: 4.1%,
Stacks: 1169, SVG size: 772K

🔥 Open interactive Flamegraph (SVG)

Flamegraph preview

📥 Download from artifacts →

🔝 Top 10 leaf functions
Function Samples
_raw_spin_unlock_irqrestore 321884112
do_syscall_64 110893002
__cp_end 91265922
srso_alias_safe_ret 43179576
x64_sys_call 26496558
find_get_block_common 25515204
dircache_process_deferred_chain 23552496
__syscall_cp_c 22571142
tcp_write_xmit 17664372
kmem_cache_free 17664372

@rdmark rdmark changed the title uams: harden DHX2, DHX, SRP key exchange and session verifier uams: introduced static library with common auth functions May 14, 2026
@rdmark
rdmark marked this pull request as draft May 14, 2026 20:30
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.

2 participants