Skip to content

Create copies of mutable properties on X509Certificate2#38884

Merged
bartonjs merged 6 commits into
dotnet:masterfrom
vcsjones:fix-38864
Jul 10, 2020
Merged

Create copies of mutable properties on X509Certificate2#38884
bartonjs merged 6 commits into
dotnet:masterfrom
vcsjones:fix-38864

Conversation

@vcsjones

@vcsjones vcsjones commented Jul 7, 2020

Copy link
Copy Markdown
Member

Export(X509ContentType.Cert) on macOS was returning the byte array from a field in the PAL, which allowed callers to mutate the underlying certificate.

IssuerName and SubjectName return mutable data on RawData, so the get there returns a copy.

Fixes #38864.

vcsjones added 2 commits July 7, 2020 13:29
Export(Cert) returned the original byte array from the PAL. If a
caller mutated the result of the export, they would be mutating the
underlying representation of RawData in the PAL. To be consistent
with Windows and Linux, we return a copy in the PAL.
@ghost

ghost commented Jul 7, 2020

Copy link
Copy Markdown

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq
Notify danmosemsft if you want to be subscribed.

Comment thread src/libraries/System.Security.Cryptography.X509Certificates/tests/CertTests.cs Outdated
@vcsjones

vcsjones commented Jul 8, 2020

Copy link
Copy Markdown
Member Author

Not sure what happened to the build, seems wedged? Should probably try (Libraries Test Build Windows_NT x64 Debug) again.

@bartonjs bartonjs closed this Jul 8, 2020
@bartonjs bartonjs reopened this Jul 8, 2020
@bartonjs

bartonjs commented Jul 8, 2020

Copy link
Copy Markdown
Member

Sadly, close/reopen only reran the already completed legs.

You can probably do it easier than I can...

git fetch --all
git merge dotnet/master
git push origin HEAD

What's that? New commit? Let's rerun all the jobs.

@vcsjones

vcsjones commented Jul 8, 2020

Copy link
Copy Markdown
Member Author

You can probably do it easier than I can

Yeah I was just hoping to avoid running the other 99 checks again and whatever carbon footprint comes along with it.

Since y'all squash merge I just threw on an empty commit.

@bartonjs

bartonjs commented Jul 8, 2020

Copy link
Copy Markdown
Member

Yeah I was just hoping to avoid running the other 99 checks again and whatever carbon footprint comes along with it.

Praiseworthy 😄. If there's an abort button somewhere in AzDO, I haven't been able to find it or can't find it because I'm not empowered to use it. Probably PEBKAC/RTFM, but is what it is 🤷.

@vcsjones

vcsjones commented Jul 8, 2020

Copy link
Copy Markdown
Member Author

Seems like AzDO had a bad day today. I'll try again tomorrow.

@safern safern mentioned this pull request Jul 8, 2020
@vcsjones

vcsjones commented Jul 9, 2020

Copy link
Copy Markdown
Member Author

macOS failure is #38998. WASM failure was System.Linq.Parallel.Tests.ToLookupTests.ToLookup_AggregateException, I'd put money on that not being related to this PR.

@bartonjs bartonjs merged commit eda20d0 into dotnet:master Jul 10, 2020
@vcsjones vcsjones deleted the fix-38864 branch July 10, 2020 15:29
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Export certificate fails under some circumstances

3 participants