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.

Sign up for GitHub

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

Add exceptions to TAR extended attribute constructors to disallow '=' and '\n' characters in keys and values. #82812

Merged
merged 5 commits into from
Apr 24, 2023
Merged

Add exceptions to TAR extended attribute constructors to disallow '=' and '\n' characters in keys and values. #82812

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b4421a6
Prevent adding extended attribute keys containing '=', '\n', or value…
carlossanlop Mar 1, 2023
5cd8c1f
Add tests to verify that the PaxTarEntry and PaxGlobalExtendedAttribu…
carlossanlop Mar 1, 2023
d690c9e
Apply CR suggestions
carlossanlop Mar 2, 2023
a6ca6ae
Use enumerator to iterate items in the specified enumeration, and ins…
carlossanlop Apr 4, 2023
b247618
Change IndexOf with Contains
carlossanlop Apr 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/libraries/System.Formats.Tar/src/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,4 +264,10 @@
<data name="TarSizeFieldTooLargeForEntryFormat" xml:space="preserve">
<value>The value of the size field for the current entry of format '{0}' is greater than the format allows.</value>
</data>
</root>
<data name="TarExtAttrDisallowedKeyChar" xml:space="preserve">
<value>The extended attribute key '{0}' contains a disallowed '{1}' character.</value>
</data>
<data name="TarExtAttrDisallowedValueChar" xml:space="preserve">
<value>The value of the extended attribute key '{0}' contains a disallowed '{1}' character.</value>
</data>
</root>
22 changes: 21 additions & 1 deletion src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,27 @@ internal TarHeader(TarEntryFormat format, TarEntryType typeFlag, TarHeader other
internal void InitializeExtendedAttributesWithExisting(IEnumerable<KeyValuePair<string, string>> existing)
{
Debug.Assert(_ea == null);
_ea = new Dictionary<string, string>(existing);
Debug.Assert(existing != null);

using IEnumerator<KeyValuePair<string, string>> enumerator = existing.GetEnumerator();
while (enumerator.MoveNext())
{
KeyValuePair<string, string> kvp = enumerator.Current;

int index = kvp.Key.IndexOfAny(new char[] { '=', '\n' });
if (index >= 0)
{
throw new ArgumentException(SR.Format(SR.TarExtAttrDisallowedKeyChar, kvp.Key, kvp.Key[index] == '\n' ? "\\n" : kvp.Key[index]));
}
if (kvp.Value.IndexOf('\n') != -1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

readability:

Suggested change
if (kvp.Value.IndexOf('\n') != -1)
if (kvp.Value.Contains('\n'))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it's more readable, but I am unsure if Contains is more or less performant compared to IndexOf. Let me find out.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Contains is strictly better in this case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually have an analyzer for this:

runtime/eng/CodeAnalysis.src.globalconfig

Lines 622 to 623 in 9c02b25

# CA2249: Consider using 'string.Contains' instead of 'string.IndexOf'
dotnet_diagnostic.CA2249.severity = warning

Do we know why it's not firing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea. @carlossanlop can you please check?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The analyzer has a bug: it only runs when using == or >=. My code is using !=.

I opened an issue: dotnet/roslyn-analyzers#6587

{
throw new ArgumentException(SR.Format(SR.TarExtAttrDisallowedValueChar, kvp.Key, "\\n"));
}

_ea ??= new Dictionary<string, string>();

_ea.Add(kvp.Key, kvp.Value);
}
}

private static string GetMagicForFormat(TarEntryFormat format) => format switch
Expand Down
27 changes: 25 additions & 2 deletions src/libraries/System.Formats.Tar/tests/TarEntry/PaxTarEntry.Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.IO;
using System.Linq;
using System.Collections.Generic;
using Xunit;

namespace System.Formats.Tar.Tests
Expand Down Expand Up @@ -36,6 +35,30 @@ public void Constructor_UnsupportedEntryTypes()
Assert.Throws<ArgumentException>(() => new PaxTarEntry(TarEntryType.GlobalExtendedAttributes, InitialEntryName));
}


[Theory]
[InlineData("\n", "value")]
[InlineData("=", "value")]
[InlineData("key", "\n")]
[InlineData("\nkey", "value")]
[InlineData("k\ney", "value")]
[InlineData("key\n", "value")]
[InlineData("=key", "value")]
[InlineData("ke=y", "value")]
[InlineData("key=", "value")]
[InlineData("key", "\nvalue")]
[InlineData("key", "val\nue")]
[InlineData("key", "value\n")]
[InlineData("key=", "value\n")]
[InlineData("key\n", "value\n")]
public void Disallowed_ExtendedAttributes_SeparatorCharacters(string key, string value)
{
Dictionary<string, string> extendedAttribute = new Dictionary<string, string>() { { key, value } };

Assert.Throws<ArgumentException>(() => new PaxTarEntry(TarEntryType.RegularFile, InitialEntryName, extendedAttribute));
Assert.Throws<ArgumentException>(() => new PaxGlobalExtendedAttributesTarEntry(extendedAttribute));
}

[Fact]
public void SupportedEntryType_RegularFile()
{
Expand Down