src: safely clear certain buffers

[vszakats]

That may hold credentials or other sensitive data, or where we want to
ensure the zeroing is not optimized out by the compiler.

Credits-to: Daniel Gustafsson
Ref: #13589 (original attempt)
Ref: #21588

Follow-up to #21645
Follow-up to 066478f #21598

---

• rebase on src: improve curlx_memzero() internal APIs #21645.

[Copilot]

Pull request overview

This PR reduces residual sensitive data in libcurl by using secure zero-before-free helpers for selected password and credential buffers.

Changes:

• Replaces plain frees with zeroing frees for URL, setopt, IMAP, MQTT, and SSPI password-related buffers.
• Adds stored credential buffer sizing so refcounted credentials can be wiped before final free.
• Tracks SSPI password length for wiping converted and identity password buffers.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
lib/urlapi.c	Zeroes URL password storage when clearing or replacing URL parts.
lib/setopt.c	Zeroes previous parsed password storage in the userpwd helper.
lib/mqtt.c	Zeroes the temporary MQTT CONNECT packet before freeing.
lib/imap.c	Zeroes the atomized IMAP LOGIN password after sending.
lib/curl_sspi.c	Zeroes temporary and identity SSPI password buffers.
lib/creds.h	Adds credential buffer size metadata.
lib/creds.c	Stores credential buffer size and zeroes credentials on final unlink.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[vszakats]

This is fine for now.

[vszakats]

That's also fine for now.

[dfandrich]

I don't like how curlx_freezero() requires tracking the allocated buffer size and keeping it around. It's too easy for this to become wrong, both when it's written but especially over time. And when it's wrong, the results can be worse than not having this feature at all. An alternative is to use malloc_usable_size() or _msize() depending on the platform to obtain it when it's needed. Since this is "just" a security-in-depth feature, platforms without this feature will still work, just without these buffers getting cleared.

[vszakats]

I don't like how curlx_freezero() requires tracking the allocated buffer size and keeping it around. It's too easy for this to become wrong, both when it's written but especially over time. And when it's wrong, the results can be worse than not having this feature at all. An alternative is to use malloc_usable_size() or _msize() depending on the platform to obtain it when it's needed. Since this is "just" a security-in-depth feature, platforms without this feature will still work, just without these buffers getting cleared.

I wonder if it would be worth the complexity? It's another two
non-portable functions to detect and roll, and deal with them
missing (AFAICS e.g. on Windows).

This to automate the 3-4 cases in this patch, vs. the 15 that
is nul-terminated. Figuring out the former is a relatively light
effort IMO. The size is available in most cases for binary buffers,
and not a bad thing to add where it happens to be missing.

[dfandrich]

I wonder if it would be worth the complexity?

That's the question. That will partly depend on how such many call sites you anticipate seeing once the entire code base is fixed.

deal with them missing (AFAICS e.g. on Windows).

_msize() should be available on Windows.

[vszakats]

deal with them missing (AFAICS e.g. on Windows).
_msize() should be available on Windows.

I still sense a deep rabbit hole here. How do we know
the memory was allocated by the allocators _msize()
expects?

[dfandrich]

I was thinking of the case where curlx_malloc and curlx_free map to the system's malloc and free directly, but you're right, if the user modifies the memory callbacks we can't make that assumption. We don't want to lose this memory safety in that case, so I guess this idea is a non-starter.

[jay]

I do a double take when I see the names like curlx_freezero, curlx_freezeroz. rather than combine the zeroing I think it would be easier to read like
curlx_zero_string(password);
curlx_free(password)

curlx_zero_mem(whatever, len);
curlx_free(whatever);

[bagder]

I'm with @jay: I prefer to have them as two separate calls - first clear, then free. I think that will also help us from overusing these calls.

[vszakats]

Main reason for adding the wrappers was to avoid the extra NULL check everywhere.
Surely doable everywhere, but it was adding noise I did not like.

edit: Though also pbly solvable by making curlx_memzero() do the check automatically.
edit2: That's not really good, because it makes an inline zeroing a call. Unless done as a macro.. I'll play with this a little.

[vszakats]

Reshuffled to drop the wrappers and offer curlx_memzero() (for raw buffers)
and curlx_strzero() (for null-terminated strings), both with built-in NULL-checks.

[bagder]

Questionable use with added complexity. Seem to not move forward. Close?

[Copilot]

🟢 Ready to approve

The changes are localized to cleanup/error paths, are NULL-safe via curlx_memzero/strzero, and the updated allocation-size tracking ensures full credential buffers are wiped without altering functional control flow.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 0 new
• Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

There is at least one error-cleanup path that still frees an existing stored password without securely clearing it first (lib/urlapi.c), which conflicts with the PR’s hardening goal.

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

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Comments suppressed due to low confidence (1)

lib/urlapi.c:350

• In parse_hostname_login()'s error cleanup path, u->password is freed via curlx_safefree() without being securely cleared first. If u->password already contained a password from a previous successful parse, this failure path will free it without zeroing, leaving the old credential in heap memory contrary to the goal of this PR.

  curlx_strzero(passwdp);
  curlx_free(passwdp);
  curlx_free(optionsp);
  curlx_safefree(u->user);
  curlx_safefree(u->password);

• Files reviewed: 11/11 changed files
• Comments generated: 0 new
• Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.