Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
nazarewk opened this issue May 29, 2024 · 7 comments
Open
Labels
Needs: Attention 👋 priority:p1 High priority/Major issue but not blocking or Big percentage of customers affected.Bug SLA <=7days ToTriage type:bug A broken experience

Comments

@nazarewk
Copy link

nazarewk commented May 29, 2024

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:

  1. which gets written to /etc/lsb-release and /etc/os-release
  2. 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))
  3. which throws: Request headers must contain only ASCII characters.

related:

@nazarewk
Copy link
Author

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

@nazarewk nazarewk changed the title Lack of non-ASCII characters escaping in HTTP Headers Lack of HTTP headers sanitization in msgraph-cli, most likely .NET Core wide May 29, 2024
nazarewk added a commit to nazarewk-iac/nix-configs that referenced this issue May 29, 2024
@nazarewk nazarewk changed the title Lack of HTTP headers sanitization in msgraph-cli, most likely .NET Core wide [BUG]: missing HTTP headers sanitization in msgraph-cli/whole .NET Core May 29, 2024
nazarewk added a commit to nazarewk-iac/nix-configs that referenced this issue May 29, 2024
this version is finally working

fixes issues with lack of HTTP header sanitization in .NET Core, see:
- NixOS/nixpkgs#315574
- microsoftgraph/msgraph-cli#477

Signed-off-by: Krzysztof Nazarewski <[email protected]>
@calebkiage
Copy link
Contributor

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
Copy link
Author

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
Copy link
Author

nazarewk commented May 29, 2024

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
Copy link

Related:
microsoft/kiota#2056
microsoft/kiota-java#126

@andrueastman Shall we implement the escaping across all languages?

@m-redding
Copy link

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

@andrueastman
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Attention 👋 priority:p1 High priority/Major issue but not blocking or Big percentage of customers affected.Bug SLA <=7days ToTriage type:bug A broken experience
Projects
None yet
Development

No branches or pull requests

5 participants