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

Pax Headers incorrectly treated is invalid if value contains an '=' sign #81699

Closed
Danielku15 opened this issue Feb 6, 2023 · 3 comments · Fixed by #82810
Closed

Pax Headers incorrectly treated is invalid if value contains an '=' sign #81699

Danielku15 opened this issue Feb 6, 2023 · 3 comments · Fixed by #82810
Assignees
Milestone

Comments

@Danielku15
Copy link

Danielku15 commented Feb 6, 2023

Description

The current TarReader treats any extended headers which contain an equals sign as invalid. While I am not sure about the exact spec, it is in conflict with the MSWINDOWS.rawsd extension which is used to define the Windows Security Descriptors for files. They are supposed to be encoded as base64 which can have = signs as padding characters.

Reproduction Steps

using System.Formats.Tar;

using var data = new MemoryStream();
using (var writer = new TarWriter(data, TarEntryFormat.Pax, leaveOpen: true))
{
    writer.WriteEntry(new PaxTarEntry(TarEntryType.RegularFile, "file.txt", new Dictionary<string, string>
    {
        ["MSWINDOWS.rawsd"] = "AQAAgBQAAAAkAAAAAAAAAAAAAAABAgAAAAAABSAAAAAhAgAAAQIAAAAAAAUgAAAAIQIAAA=="
    })
    {
        DataStream = new MemoryStream("Hello Pax"u8.ToArray())
    });
}

data.Position = 0;
using (var reader = new TarReader(data, leaveOpen: true))
{
    var entry = (PaxTarEntry)reader.GetNextEntry();
    if (!entry.ExtendedAttributes.TryGetValue("MSWINDOWS.rawsd", out var descriptor))
    {
        descriptor = "missing!";
    }
    
    Console.WriteLine("Security Descriptor: {0}", descriptor);
}

Expected behavior

Output should show Security Descriptor: AQAAgBQAAAAkAAAAAAAAAAAAAAABAgAAAAAABSAAAAAhAgAAAQIAAAAAAAUgAAAAIQIAAA==

Actual behavior

Output shows Security Descriptor: missing!

Regression?

No, was like this since the beginning.

Known Workarounds

None known.

Configuration

.net Version: 7.0.102

Edition	Windows 11 Enterprise
Version	22H2
Installed on	‎25/‎01/‎2023
OS build	22621.1105
Experience	Windows Feature Experience Pack 1000.22638.1000.0

Architecture: x64

Other information

It currently seems by design to treat equals sign values as invalid even though it shouldn't be a problem to support it. From the remaining parsing code it seems to be expected that properties are separated by newlines and key-value by the first equals sign.

Related code:

// If the value contains an =, it's malformed.
if (valueSlice.IndexOf((byte)'=') >= 0)
{
return false;
}

I was extending https://github.com/dotnet/sdk-container-builds with features to support Windows Containers better and ran into this problem while creating unit tests to verify that the extended attributes are written.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Feb 6, 2023
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@carlossanlop carlossanlop self-assigned this Feb 13, 2023
@carlossanlop carlossanlop added this to the 8.0.0 milestone Feb 13, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Feb 13, 2023
@carlossanlop
Copy link
Member

Thanks for reporting, @Danielku15. I agree it's a bug. I could not find a spec explicitly forbidding the '=' character. The only character that should be forbidden as part of a value is '\n' because it is used as separator of the key value pairs.

I have prepared a fix for main and will submit a PR soon. I assume the sdk-container-builds needs a backport for 7.0, correct?

@Danielku15
Copy link
Author

I assume the sdk-container-builds needs a backport for 7.0, correct?

@carlossanlop Yes, that would be beneficial. There are extensions planned for 7.0.300 which require currently the certain checks in the unit tests to be disabled. This implies a risk of regression bugs of the related features.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Mar 9, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Apr 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants