[BUG]: missing HTTP headers sanitization in `msgraph-cli`/whole .NET Core

nazarewk commented 3 months ago

I don't want to spend a day trying to find the right place to file the bug, but looks like whatever msgraph-cli uses does sanitize the HTTP Header values properly.

Latest version of NixOS has a unicode character in release codename Vicuña:

which gets written to /etc/lsb-release and /etc/os-release
which gets read by the library into HTTP Header as-is without any sanitization: User-Agent:azsdk-net-Identity/1.10.4 (.NET 8.0.5; NixOS 24.11 (Vicuña))
which throws: Request headers must contain only ASCII characters.

nazarewk commented 3 months ago

I'm seeing others reporting issues with other .NET tools on the Nix community chat

calebkiage commented 3 months ago

Hi, thanks for raising this issue. Since this affects all .NET programs, maybe reporting it in the .NET repository can also get the team's attention. See dotnet/sdk

What do other programs like curl and the browser do with the user agent string on NixOS (I've never used this distro)? I wouldn't want to add header sanitization to this CLI since there's no agreed upon protocol as far as I can tell for doing this reliably. i.e. if I encode a header value, will the server decode it reliably and get the same data I encoded? We also cannot add the specific scenario since that's not scalable. I think the best way forward would be to somehow update these values on the OS config and remove/replace the unicode characters.

nazarewk commented 3 months ago

NixOS is not doing anything special in this regard, there were other distros with unicode characters in release names.

I might be missing something, but there are RFC 5987 and RFC2231 before that defining what should happen: basically urlencode the values.

I am pretty sure (last time I checked/was concerned with it was around 2016) web browsers are doing just this.

As far as I remember Python's requests library does this out of the box too and I guess many other libraries.

curl does not try to read random values from the system that it has no influence over, unlike .NET Core it seems.

I can easily imagine automatically (and unconditionally) reading a value directly into HTTP header being some kind of attack vector.

httpbin seems to handle non-ASCII/UTF-8 characters just fine:

> curl 'https://httpbin.org/get' -H 'Test-Header: Vicuña'
{
  "args": {}, 
  "headers": {
    "Accept": "*/*", 
    "Host": "httpbin.org", 
    "Test-Header": "Vicu\u00c3\u00b1a", 
    "User-Agent": "curl/8.7.1", 
    "X-Amzn-Trace-Id": "Root=1-66572d1f-2bc57bbd6292abcd1b93b473"
  }, 
  "origin": "95.168.118.8", 
  "url": "https://httpbin.org/get"
}

nazarewk commented 3 months ago

I'm not actually sure where does it take the Vicuna string from , the source code seems to readonly ID and VERSION_ID, none of those contain the codename, the string in the HTTP header seems to be PRETTY_NAME:

> cat /etc/os-release
ANSI_COLOR="1;34"
BUG_REPORT_URL="https://github.com/NixOS/nixpkgs/issues"
BUILD_ID="24.11.20240528.aae38d0"
DOCUMENTATION_URL="https://nixos.org/learn.html"
HOME_URL="https://nixos.org/"
ID=nixos
IMAGE_ID=""
IMAGE_VERSION=""
LOGO="nix-snowflake"
NAME=NixOS
PRETTY_NAME="NixOS 24.11 (Vicuna)"
SUPPORT_URL="https://nixos.org/community.html"
VERSION="24.11 (Vicuna)"
VERSION_CODENAME=vicuna
VERSION_ID="24.11"

edit: this search seems to give some clue https://github.com/search?q=org%3Adotnet+%22PRETTY_NAME%22+language%3AC%23&type=code

petrhollayms commented 3 months ago

@andrueastman Shall we implement the escaping across all languages?

m-redding commented 3 months ago

This has been fixed in https://github.com/Azure/azure-sdk-for-net/pull/44386 and will be released in Azure.Core 1.40.

andrueastman commented 3 months ago

This has been fixed in Azure/azure-sdk-for-net#44386 and will be released in Azure.Core 1.40.

Thanks @m-redding

The user agent header sent by Kiota libraries does not include the OS information (only the language and version of kiota). So, the header here is sent by the Azure library. Once we update the azure dependency to 1.40, we should retest and evaluate whether the header is cleaned up.

microsoftgraph / msgraph-cli

[BUG]: missing HTTP headers sanitization in `msgraph-cli`/whole .NET Core #477