Skip to content

fix: keep GnuTLS and NSS probing enabled by default in sslsniff - #227

Open
yunwei37 wants to merge 6 commits into
mainfrom
fix/sslsniff-provider-defaults
Open

fix: keep GnuTLS and NSS probing enabled by default in sslsniff#227
yunwei37 wants to merge 6 commits into
mainfrom
fix/sslsniff-provider-defaults

Conversation

@yunwei37

@yunwei37 yunwei37 commented Sep 4, 2026

Copy link
Copy Markdown
Member

Since #164 (337af70), the default provider flags for the sslsniff example are gnutls = false and nss = false. The command line only offers --no-gnutls and --no-nss, which clear those same flags, so on current main there is no way to enable GnuTLS or NSS probing at all. That contradicts the documented default usage (./sslsniff # sniff OpenSSL and GnuTLS functions), and applications that use GnuTLS or NSS are not captured even when the libraries are installed.

Changes:

  • Restore gnutls = true and nss = true as the defaults in src/30-sslsniff/sslsniff.c.
  • When a provider library is not found via ldconfig, print a warning to stderr and skip that provider instead of passing a NULL path into the uprobe attach.
  • Match SONAME prefixes literally when resolving providers through ldconfig.
  • Retain and clean up every successful uprobe link, report partial provider attachment, and fail when no provider attaches.
  • Sync the lesson-30 README code excerpts (English and Chinese) with the current provider-attach block.

Validation:

  • make -B -C src/30-sslsniff builds the BPF object, skeleton, and user-space binary.
  • The 13 source-block synchronization tests pass.
  • All relative Markdown links resolve.
  • Live TLS attachment was not exercised in this container because it has no kernel BPF capability; repository CI covers the privileged run.

Related to #121 and #99.

Commit 337af70 (add hook of ssl_ex version) flipped the default provider
flags to off. The --no-gnutls and --no-nss options only clear the same
flags, so GnuTLS and NSS probing became unreachable and the documented
default behavior (sniff OpenSSL and GnuTLS functions) stopped holding.

Restore the gnutls/nss defaults to on, and warn instead of attaching
against a NULL library path when a provider library is not present on
the system.
Copilot AI lite review requested due to automatic review settings September 4, 2026 00:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new/updated provider path lookups still pass unescaped patterns into a grep-based resolver, which can produce non-literal matches and should be corrected to avoid selecting unintended libraries.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes the sslsniff example’s provider enablement defaults so GnuTLS and NSS probing are enabled again by default, and it avoids attempting uprobe attaches when a provider library can’t be located via ldconfig, aligning runtime behavior with the documented intended usage.

Changes:

  • Restore default provider flags to enable GnuTLS and NSS probing.
  • Skip provider attach when find_library_path() returns NULL, emitting a warning instead of passing a NULL path into uprobe attach.
File summaries
File Description
src/30-sslsniff/sslsniff.c Re-enables default providers and adds NULL-path guarding with warnings before attaching provider uprobes.
Review details

Suppressed comments (2)

src/30-sslsniff/sslsniff.c:419

  • find_library_path() runs grep in basic-regex mode; the dot in this pattern is a wildcard. Escaping it avoids accidental matches against unrelated library names.
		char *nss_path = find_library_path("libnspr4.so");

src/30-sslsniff/sslsniff.c:410

  • find_library_path() uses grep with basic regex, so the dot in libgnutls.so is treated as a wildcard. Escape it to make the match literal (or change find_library_path() to use grep -F).
		char *gnutls_path = find_library_path("libgnutls.so");
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/30-sslsniff/sslsniff.c Outdated
Update the lesson-30 README code excerpts (English and Chinese) to the
current sslsniff.c provider-attach block, which now warns and skips a
provider whose library is not present instead of attaching against a
NULL path.
@pull-request-size pull-request-size Bot added size/M and removed size/S labels Sep 4, 2026
@pull-request-size pull-request-size Bot added size/L and removed size/M labels Sep 4, 2026
@yunwei37

yunwei37 commented Sep 4, 2026

Copy link
Copy Markdown
Member Author

Final exact-head review for 9ca1d31 is complete.

  • Two independent review passes found no blocking correctness, ownership, scope, documentation, or maintainability issue.
  • The final PR diff is limited to lesson 30: +79/-64 across sslsniff.c and the matching English/Chinese READMEs, net +15. Production code is net +33 while documentation is net -18; there is no test-fixture growth. The added lines implement necessary link ownership and provider handling, while the final helper refactor removes repeated branches from main.
  • Review verified literal SONAME lookup, libbpf error handling, exact-once link cleanup after partial attachment, graceful missing-provider behavior, failure when no probe attaches, restored GnuTLS/NSS defaults, help text, bilingual source excerpts, and issue relationships.
  • The lesson build, syntax check, 13 source-block synchronization checks, internal-link validation, diff checks, and every exact-head GitHub check passed. The sole Copilot finding is answered and resolved.

Ready for maintainer review; no merge was performed.

AI-generated response; a maintainer will review and follow up later

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants