idn: restore MultiByteToWideChar() MB_ERR_INVALID_CHARS flag

[vszakats]

Also:

• curlx: pass this flag to the actual conversion calls, for consistency
  and robustness. (It's not stricly necessary because the initial call
  to determine size, with this flag passed, fails already on bad input.)
• schannel: unfold MultiByteToWideChar() line (formatting).

Ref: https://learn.microsoft.com/windows/win32/api/stringapiset/nf-stringapiset-multibytetowidechar

Follow-up to 6694a42 #19798

[Copilot]

Pull request overview

This PR restores use of the Windows MultiByteToWideChar() MB_ERR_INVALID_CHARS flag to ensure invalid UTF-8 input is rejected during IDN (punycode) conversions, and aligns related UTF-8→wchar conversion helpers for consistent validation behavior across Windows codepaths.

Changes:

• Use MB_ERR_INVALID_CHARS in Win32 IDN UTF-8→UTF-16 conversions so invalid UTF-8 fails fast (lib/idn.c).
• Pass MB_ERR_INVALID_CHARS consistently to the actual conversion calls (not only the sizing call) in curlx UTF-8→wchar helpers (lib/curlx/multibyte.c, lib/curlx/fopen.c).
• Minor formatting adjustment to a MultiByteToWideChar() call in Schannel client cert import (lib/vtls/schannel.c).

Reviewed changes

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

File	Description
lib/vtls/schannel.c	Formatting-only tweak around MultiByteToWideChar() usage when converting the PKCS#12 password.
lib/idn.c	Restores MB_ERR_INVALID_CHARS for Win32 IDN conversions to reject invalid UTF-8 input.
lib/curlx/multibyte.c	Ensures the conversion call itself uses MB_ERR_INVALID_CHARS (matching the sizing call).
lib/curlx/fopen.c	Same consistency hardening for UTF-8→wchar conversion used by Windows fopen helpers.

---

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

[jay]

I don't have any objections to this but thinking it through if fopen is called with some output filename that is not UTF-8 (like maybe an old local encoded content disposition from the server?), and so it can't convert it, then this change would make it error rather than using placeholder characters. Do you think this scenario is plausible, could it create friction for users who really don't know about encoding etc but just want the file saved regardless of encoding mismatch?

[vszakats]

fopen() already called the conversion with this flag (in the size determination
step), this patch does not change it. What this patch does is pass the flag to the
second conversion invocation doing the actual conversion. It doesn't change
functionality, but perhaps makes the intent clearer and avoids accidentally
dropping the flag when reusing this code.

As for IDN, it restores what has been there before my patch a few versions ago
where I accidentally dropped the flag.

I think it's probably better to reject bad input early, as this perhaps avoids letting
weird data reach the rest of code, and weirdly encoded data may be a safety issue,
with potential exploits. I did not think very deeply about it, because this is how
curl worked for a good while now.

There is one more place (3 calls) in src/tool_cb_wrt.c that do not pass this
flag. I was wondering if it'd make sense to set it there too. It's in the output
dumping code, so the security impact seems to fall outside of curl, if any.

[jay]

What this patch does is pass the flag to the
second conversion invocation doing the actual conversion. It doesn't change
functionality, but perhaps makes the intent clearer and avoids accidentally
dropping the flag when reusing this code.

Ok

There is one more place (3 calls) in src/tool_cb_wrt.c that do not pass this
flag. I was wondering if it'd make sense to set it there too. It's in the output
dumping code, so the security impact seems to fall outside of curl, if any.

Those calls are to convert what we assume is UTF-8 encoded Unicode content from the server to the user's terminal. I disagree and think we should continue to print placeholder characters when content can't be converted, rather than error. Some servers aren't going to meet the assumption. Personally I'd rather see the content and I think from the user perspective they probably would too rather than see an error.

[vszakats]

What this patch does is pass the flag to the
second conversion invocation doing the actual conversion. It doesn't change
functionality, but perhaps makes the intent clearer and avoids accidentally
dropping the flag when reusing this code.

Ok

There is one more place (3 calls) in src/tool_cb_wrt.c that do not pass this
flag. I was wondering if it'd make sense to set it there too. It's in the output
dumping code, so the security impact seems to fall outside of curl, if any.

Those calls are to convert what we assume is UTF-8 encoded Unicode content from the server to the user's terminal. I disagree and think we should continue to print placeholder characters when content can't be converted, rather than error. Some servers aren't going to meet the assumption. Personally I'd rather see the content and I think from the user perspective they probably would too rather than see an error.

I agree, yes, it's better not to throw away the whole chunk just because
there are some invalids in it. Though the UTF-8 detection is itself lossy /
heuristic and the conversion will possibly be also lossy for certain payloads
if we convert to UTF-16.

Something to perhaps consider is to let is passthrough as-is if the OS is
capable of accepting it. (and if there is way to detect this.)