Fix <img /> detection regex in web cmdlets #12099

vexx32 · 2020-03-11T01:10:40Z

PR Summary

In c64a28e, the img tag regex was changed to <img\s+[^\s>]*> to avoid a DoS issue.

This regex will not match common image tags that contain more than just the src attribute (e.g., <img src="$url" class="className"/>

Fix is to adjust the regex to not preclude spaces after other content in the tag.

PR Context

Resolves #12089

PR Checklist

...hell.Commands.Utility/commands/utility/WebCmdlet/Common/BasicHtmlWebResponseObject.Common.cs

vexx32 · 2020-03-11T02:26:24Z

@PoshChan please retry macos

PoshChan · 2020-03-11T02:26:39Z

@vexx32, successfully started retry of PowerShell-CI-macOS

iSazonov · 2020-03-11T05:52:34Z

We have 6 Regex in the file. It would nice to add tests. xUnit could be most stable.

TravisEz13 · 2020-03-11T18:44:35Z

There are no existing tests except the test to verify the last change didn't regress. We should add tests, but as this is regression, and we are early in 7.1, my question is have you manually tested it.

But to port it to 7.0, we need to add tests to cover at least a couple of cases.

here is the test I added:

PowerShell/test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

Line 1904 in 920b671

It "Image Parsing" {

vexx32 · 2020-03-11T19:12:06Z

@TravisEz13 yep I manually tested it with regex101. On a page that contains a bunch of images (literally just a page about travel blogs with a bunch of pictures), here's the breakdown:

"Pathological" regex from the test:

2ms
25 matches
1708 steps

Currently-used regex in the codebase:

~1ms
No matches
1683 steps

Proposed new regex in this PR:

1ms
25 matches
1708 steps

There may be a more effective test case available, but I just grabbed the first thing I could think of 😅

TravisEz13 · 2020-03-11T21:31:55Z

for it to be pathological, you need to test it with the pathological case (lots of spaces after img)... but there is a test case for that.

TravisEz13 · 2020-03-11T21:35:31Z

@vexx32 if you aren't planning on adding the cases, can you open an issue to add the cases and port both PR's back to 7.0 after the cases have been added.

vexx32 · 2020-03-11T22:27:24Z

I don't mind adding the cases, but I'm not really clear on where / how these cases are defined (would I need to add something to that Get-WebListenerUrl function?), or exactly what additional cases you're looking to have added here? Tests for something like <img$(" " * 5000)src="dummyimage.png"$(" " * 5000)class="classname"/>?

iSazonov · 2020-03-12T03:22:21Z

Is there a compliance analyzer tool for Regex?

iSazonov · 2020-03-12T03:32:22Z

@vexx32 I think @TravisEz13 says that we could add tests like existing one for rest 5 Regex.

TravisEz13 · 2020-03-12T16:49:04Z

@iSazonov we are supposed to use a tool in azure. It's public, but to use it free, we are supposed to use an internal version. To use it, we need a test case (or a few) where the content going to the regex is in a file (separate file per case). The tool is basically and AI and will vary the content of the file until all codepaths have been hit.

I'd really appreciate if someone added a case like this. We could remove the pathological test cases if we had cases like this and those cases are unreliable.

TravisEz13 · 2020-03-12T16:50:38Z

@vexx32 for this (#12099 (comment)) new case, we don't need to add a large number of spaces.

iSazonov · 2020-03-13T10:02:52Z

@TravisEz13 Do you ask to create one HTML test file per Regex from this 5 Regex in the PR? Should it be full featured HTML page or simple string like
<img class="image a11y-hidden" src="https://mc.yandex.ru/watch/723233"/>

TravisEz13 · 2020-03-13T18:13:46Z

@iSazonov
This is the tool: https://www.microsoft.com/en-us/security-risk-detection/
The description of the requirement are here:
https://docs.microsoft.com/en-us/security-risk-detection/concepts/seed-files

Microsoft Security Risk Detection
Microsoft Security Risk Detection
Security Risk Detection is Microsoft's unique fuzz testing service for finding security critical bugs in software, helping customers quickly adopt practices and technology battle-tested at Microsoft.

Seed Files
Guidelines and best practices to choosing the corpus of seed files to maximize fuzzing efficiency and results

iSazonov · 2020-03-13T19:49:32Z

@TravisEz13 The docs say that we could have up to 1000 seed files. I think we could create a list of sites based on different engines, grab pages from them to use as seed files. I believe it will be more productive and native than manually create HTML files.

TravisEz13 · 2020-03-16T17:12:21Z

@iSazonov The tool may allow that, but during our testing internet access is disabled. So, the test would fail. (It may not be that way today, but it will be in the future.). The whole purpose of this tool is that it does the variation so that you don't have to produce every input.

See https://en.wikipedia.org/wiki/Fuzzing

iSazonov · 2020-03-16T17:32:45Z

@TravisEz13 My suggestion is not to access Internet at test time. My suggestion is:

create a list of popular sites based on different templates (Sharepoint and so on)
download pages from the sites
use these downloaded static html pages in tests

TravisEz13 · 2020-03-16T20:35:16Z

@iSazonov in theory that works, but there are legal issues with that. Sharepoint is fine, but the other sites we may need to get the sites permission.

vexx32 · 2020-03-16T21:42:21Z

Opened #12138; feel free to continue that discussion there, as it doesn't look like this will be a simple decision of what tests to add for this PR. 🙂

TravisEz13 · 2020-03-16T21:50:31Z

@vexx32 Can you add a test like you were suggesting here: #12099 (comment) minus the spaces (that's already covered)

vexx32 · 2020-03-16T22:04:55Z

Happy to, but I'm not not clear on how cases are added here, as it looks like the content generation is handled elsewhere. Can you point me to where that is?

TravisEz13 · 2020-03-16T22:29:53Z

Here is the commit where I added the last test case and tho WebListener "Controller" that the test cases uses: c64a28e

iSazonov · 2020-03-17T03:10:00Z

in theory that works, but there are legal issues with that. Sharepoint is fine, but the other sites we may need to get the sites permission.

@TravisEz13 If I understand right you will make the tests internally so the site list will be not public. We are not going to change, share or distribute the pages. We are not going to download any artifacts like pictures - we need only html. We are not going to check these sites and distribute any information about it - we are going to test only that our code works correctly on the data.

TravisEz13 · 2020-03-18T19:11:38Z

test/tools/WebListener/Controllers/DosController.cs

                    contentType = "text/html; charset=utf8";
                    body = "<img" + (new string(' ', dosLength));
                    break;
+                case "img-attribute":


This isn't actually a DOS... can you just add a comment explaining that?

@TravisEz13 Yeah I was torn on that myself. I could also just add a new Controller class here instead, since we plan on adding future cases to test the actual regex patterns' capability to parse what we expect?

I don't think you need to. I'll leave it up to you, but at least add a comment that this is not a DOS case, but you didn't want to create a new controller for a case that was so similar.

TravisEz13 · 2020-03-18T20:57:15Z

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

            $pathologicalRatio | Should -BeGreaterThan 5
        }
+
+        It 'correctly parses an image with id, class, and src attributes' {


And you should probably move this to a new context too.

TravisEz13 · 2020-03-23T20:22:42Z

I used fixing a conflict in your PR to partially fix a mistake I made... Brought a test back, but pended it.
related to #12155

ghost · 2020-04-23T18:03:14Z

🎉v7.1.0-preview.2 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

ghost · 2020-05-14T22:52:51Z

🎉v7.0.1 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

# Conflicts: # test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

🐛 Fix img detection regex.

3971046

ghost assigned anmenaga Mar 11, 2020

PoshChan reviewed Mar 11, 2020

View reviewed changes

...hell.Commands.Utility/commands/utility/WebCmdlet/Common/BasicHtmlWebResponseObject.Common.cs Show resolved Hide resolved

TravisEz13 added this to the 7.0.x-Servicing-Consider milestone Mar 11, 2020

TravisEz13 added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Mar 11, 2020

vexx32 mentioned this pull request Mar 16, 2020

Add Regression Tests for Web Cmdlets' Regex Patterns #12138

Closed

✅ Add test case

4c25150

vexx32 requested review from TravisEz13, adityapatwardhan, anmenaga and daxian-dbw as code owners March 17, 2020 23:36

iSazonov approved these changes Mar 18, 2020

View reviewed changes

TravisEz13 requested changes Mar 18, 2020

View reviewed changes

ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Mar 18, 2020

TravisEz13 reviewed Mar 18, 2020

View reviewed changes

vexx32 added 2 commits March 21, 2020 16:41

📝 addressed review comments

bb42b21

♻️ move test to new context

daec401

TravisEz13 approved these changes Mar 23, 2020

View reviewed changes

Merge branch 'master' into Web/ImageRegex

1792218

anmenaga merged commit f8fb770 into PowerShell:master Mar 24, 2020

vexx32 deleted the Web/ImageRegex branch March 24, 2020 17:54

TravisEz13 mentioned this pull request Mar 25, 2020

Un-Pend test after NET 5 Preview 2 #12155

Closed

adityapatwardhan modified the milestones: 7.0.x-Servicing-Consider, 7.0.1-approved Apr 20, 2020

TravisEz13 modified the milestones: 7.0.1-approved, 7.0.1 Apr 20, 2020

ghost mentioned this pull request Apr 23, 2020

Invoke-WebRequest Images missing #12089

Closed

silijon pushed a commit to SkyKick/PowerShell that referenced this pull request Jul 2, 2020

Fix <img /> detection regex in web cmdlets (PowerShell#12099)

a49beb6

# Conflicts: # test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

TravisEz13 mentioned this pull request Oct 12, 2022

Back-port to 7.0: Update list of PowerShell team members in release tools #18266

Merged

22 tasks

Fix <img /> detection regex in web cmdlets #12099

Fix <img /> detection regex in web cmdlets #12099

Uh oh!

Conversation

vexx32 commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

Uh oh!

vexx32 commented Mar 11, 2020

Uh oh!

PoshChan commented Mar 11, 2020

Uh oh!

iSazonov commented Mar 11, 2020

Uh oh!

TravisEz13 commented Mar 11, 2020

Uh oh!

vexx32 commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TravisEz13 commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TravisEz13 commented Mar 11, 2020

Uh oh!

vexx32 commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Mar 12, 2020

Uh oh!

iSazonov commented Mar 12, 2020

Uh oh!

TravisEz13 commented Mar 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TravisEz13 commented Mar 12, 2020

Uh oh!

iSazonov commented Mar 13, 2020

Uh oh!

TravisEz13 commented Mar 13, 2020 • edited by unfurl-links bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Mar 13, 2020

Uh oh!

TravisEz13 commented Mar 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Mar 16, 2020

Uh oh!

TravisEz13 commented Mar 16, 2020

Uh oh!

vexx32 commented Mar 16, 2020

Uh oh!

TravisEz13 commented Mar 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vexx32 commented Mar 16, 2020

Uh oh!

TravisEz13 commented Mar 16, 2020

Uh oh!

iSazonov commented Mar 17, 2020

Uh oh!

TravisEz13 Mar 18, 2020

Choose a reason for hiding this comment

Uh oh!

vexx32 Mar 18, 2020

Choose a reason for hiding this comment

Uh oh!

TravisEz13 Mar 18, 2020

Choose a reason for hiding this comment

Uh oh!

TravisEz13 Mar 18, 2020

Choose a reason for hiding this comment

Uh oh!

TravisEz13 commented Mar 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Apr 23, 2020

vexx32 commented Mar 11, 2020 •

edited

Loading

vexx32 commented Mar 11, 2020 •

edited

Loading

TravisEz13 commented Mar 11, 2020 •

edited

Loading

vexx32 commented Mar 11, 2020 •

edited

Loading

TravisEz13 commented Mar 12, 2020 •

edited

Loading

TravisEz13 commented Mar 13, 2020 •

edited by unfurl-links bot

Loading

TravisEz13 commented Mar 16, 2020 •

edited

Loading

TravisEz13 commented Mar 16, 2020 •

edited

Loading

TravisEz13 commented Mar 23, 2020 •

edited

Loading