diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-10-27 10:19:45 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-10-27 10:20:01 +0000 |
commit | 431f1b7aa248c4e4d5f251e6ebf5f9157f9c1243 (patch) | |
tree | e0545534942176a4eea91f45db57c0f09422fc4b | |
parent | 0a9edaf533931d62656ac1e8c306a9d17a088538 (diff) | |
download | gitlab-ce-431f1b7aa248c4e4d5f251e6ebf5f9157f9c1243.tar.gz |
Add latest changes from gitlab-org/security/gitlab@14-3-stable-ee
-rw-r--r-- | app/assets/javascripts/lib/dompurify.js | 19 | ||||
-rw-r--r-- | app/assets/javascripts/lib/utils/url_utility.js | 18 | ||||
-rw-r--r-- | spec/frontend/lib/dompurify_spec.js | 6 | ||||
-rw-r--r-- | spec/frontend/lib/utils/url_utility_spec.js | 21 | ||||
-rw-r--r-- | workhorse/internal/upload/exif/testdata/takes_lot_of_memory_to_decode.tiff | bin | 0 -> 9662 bytes | |||
-rw-r--r-- | workhorse/internal/upload/rewrite.go | 2 | ||||
-rw-r--r-- | workhorse/internal/upload/rewrite_test.go | 13 |
7 files changed, 69 insertions, 10 deletions
diff --git a/app/assets/javascripts/lib/dompurify.js b/app/assets/javascripts/lib/dompurify.js index d421d66981e..47ede8cb1bb 100644 --- a/app/assets/javascripts/lib/dompurify.js +++ b/app/assets/javascripts/lib/dompurify.js @@ -1,5 +1,5 @@ import { sanitize as dompurifySanitize, addHook } from 'dompurify'; -import { getBaseURL, relativePathToAbsolute } from '~/lib/utils/url_utility'; +import { getNormalizedURL, getBaseURL, relativePathToAbsolute } from '~/lib/utils/url_utility'; const defaultConfig = { // Safely allow SVG <use> tags @@ -11,12 +11,14 @@ const defaultConfig = { // Only icons urls from `gon` are allowed const getAllowedIconUrls = (gon = window.gon) => - [gon.sprite_file_icons, gon.sprite_icons].filter(Boolean); + [gon.sprite_file_icons, gon.sprite_icons] + .filter(Boolean) + .map((path) => relativePathToAbsolute(path, getBaseURL())); -const isUrlAllowed = (url) => getAllowedIconUrls().some((allowedUrl) => url.startsWith(allowedUrl)); +const isUrlAllowed = (url) => + getAllowedIconUrls().some((allowedUrl) => getNormalizedURL(url).startsWith(allowedUrl)); -const isHrefSafe = (url) => - isUrlAllowed(url) || isUrlAllowed(relativePathToAbsolute(url, getBaseURL())) || url.match(/^#/); +const isHrefSafe = (url) => url.match(/^#/) || isUrlAllowed(url); const removeUnsafeHref = (node, attr) => { if (!node.hasAttribute(attr)) { @@ -36,13 +38,14 @@ const removeUnsafeHref = (node, attr) => { * <use href="/assets/icons-xxx.svg#icon_name"></use> * </svg> * + * It validates both href & xlink:href attributes. + * Note that `xlink:href` is deprecated, but still in use + * https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/xlink:href + * * @param {Object} node - Node to sanitize */ const sanitizeSvgIcon = (node) => { removeUnsafeHref(node, 'href'); - - // Note: `xlink:href` is deprecated, but still in use - // https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/xlink:href removeUnsafeHref(node, 'xlink:href'); }; diff --git a/app/assets/javascripts/lib/utils/url_utility.js b/app/assets/javascripts/lib/utils/url_utility.js index bca0e45d98d..deedc210e5c 100644 --- a/app/assets/javascripts/lib/utils/url_utility.js +++ b/app/assets/javascripts/lib/utils/url_utility.js @@ -397,6 +397,24 @@ export function isSafeURL(url) { } } +/** + * Returns a normalized url + * + * https://gitlab.com/foo/../baz => https://gitlab.com/baz + * + * @param {String} url - URL to be transformed + * @param {String?} baseUrl - current base URL + * @returns {String} + */ +export const getNormalizedURL = (url, baseUrl) => { + const base = baseUrl || getBaseURL(); + try { + return new URL(url, base).href; + } catch (e) { + return ''; + } +}; + export function getWebSocketProtocol() { return window.location.protocol.replace('http', 'ws'); } diff --git a/spec/frontend/lib/dompurify_spec.js b/spec/frontend/lib/dompurify_spec.js index 324441fa2c9..47a94a4dcde 100644 --- a/spec/frontend/lib/dompurify_spec.js +++ b/spec/frontend/lib/dompurify_spec.js @@ -22,12 +22,16 @@ const safeUrls = { const unsafeUrls = [ '/an/evil/url', '../../../evil/url', - 'https://evil.url/assets/icons-123a.svg', + 'https://evil.url/assets/icons-123a.svg#test', 'https://evil.url/assets/icons-456b.svg', `https://evil.url/${rootGon.sprite_icons}`, `https://evil.url/${rootGon.sprite_file_icons}`, `https://evil.url/${absoluteGon.sprite_icons}`, `https://evil.url/${absoluteGon.sprite_file_icons}`, + `${rootGon.sprite_icons}/../evil/path`, + `${rootGon.sprite_file_icons}/../../evil/path`, + `${absoluteGon.sprite_icons}/../evil/path`, + `${absoluteGon.sprite_file_icons}/../../https://evil.url`, ]; const forbiddenDataAttrs = ['data-remote', 'data-url', 'data-type', 'data-method']; diff --git a/spec/frontend/lib/utils/url_utility_spec.js b/spec/frontend/lib/utils/url_utility_spec.js index 6f186ba3227..74855524809 100644 --- a/spec/frontend/lib/utils/url_utility_spec.js +++ b/spec/frontend/lib/utils/url_utility_spec.js @@ -607,6 +607,27 @@ describe('URL utility', () => { }); }); + describe('getNormalizedURL', () => { + it.each` + url | base | result + ${'./foo'} | ${''} | ${'http://test.host/foo'} + ${'../john.md'} | ${''} | ${'http://test.host/john.md'} + ${'/images/img.png'} | ${'https://gitlab.com'} | ${'https://gitlab.com/images/img.png'} + ${'/images/../img.png'} | ${'https://gitlab.com'} | ${'https://gitlab.com/img.png'} + ${'/images/./img.png'} | ${'https://gitlab.com'} | ${'https://gitlab.com/images/img.png'} + ${'./images/img.png'} | ${'https://gitlab.com/user/project'} | ${'https://gitlab.com/user/images/img.png'} + ${'../images/../img.png'} | ${'https://gitlab.com/user/project'} | ${'https://gitlab.com/img.png'} + ${'/images/img.png'} | ${'https://gitlab.com/user/project'} | ${'https://gitlab.com/images/img.png'} + ${'/images/../img.png'} | ${'https://gitlab.com/user/project'} | ${'https://gitlab.com/img.png'} + ${'/images/./img.png'} | ${'https://gitlab.com/user/project'} | ${'https://gitlab.com/images/img.png'} + `( + 'converts url "$url" with base "$base" to normalized url => "expected"', + ({ url, base, result }) => { + expect(urlUtils.getNormalizedURL(url, base)).toBe(result); + }, + ); + }); + describe('getWebSocketProtocol', () => { it.each` protocol | expectation diff --git a/workhorse/internal/upload/exif/testdata/takes_lot_of_memory_to_decode.tiff b/workhorse/internal/upload/exif/testdata/takes_lot_of_memory_to_decode.tiff Binary files differnew file mode 100644 index 00000000000..6935cb130db --- /dev/null +++ b/workhorse/internal/upload/exif/testdata/takes_lot_of_memory_to_decode.tiff diff --git a/workhorse/internal/upload/rewrite.go b/workhorse/internal/upload/rewrite.go index 79ebfe950c5..3dfab120188 100644 --- a/workhorse/internal/upload/rewrite.go +++ b/workhorse/internal/upload/rewrite.go @@ -233,7 +233,7 @@ func handleExifUpload(ctx context.Context, r io.Reader, filename string, imageTy } func isTIFF(r io.Reader) bool { - _, err := tiff.Decode(r) + _, err := tiff.DecodeConfig(r) if err == nil { return true } diff --git a/workhorse/internal/upload/rewrite_test.go b/workhorse/internal/upload/rewrite_test.go index 6fc41c3fefd..e3f33a02489 100644 --- a/workhorse/internal/upload/rewrite_test.go +++ b/workhorse/internal/upload/rewrite_test.go @@ -2,6 +2,7 @@ package upload import ( "os" + "runtime" "testing" "github.com/stretchr/testify/require" @@ -29,6 +30,10 @@ func TestImageTypeRecongition(t *testing.T) { filename: "exif/testdata/sample_exif_invalid.jpg", isJPEG: false, isTIFF: false, + }, { + filename: "exif/testdata/takes_lot_of_memory_to_decode.tiff", // File from https://gitlab.com/gitlab-org/gitlab/-/issues/341363 + isJPEG: false, + isTIFF: true, }, } @@ -36,8 +41,16 @@ func TestImageTypeRecongition(t *testing.T) { t.Run(test.filename, func(t *testing.T) { input, err := os.Open(test.filename) require.NoError(t, err) + + var m runtime.MemStats + runtime.ReadMemStats(&m) + start := m.TotalAlloc + require.Equal(t, test.isJPEG, isJPEG(input)) require.Equal(t, test.isTIFF, isTIFF(input)) + + runtime.ReadMemStats(&m) + require.Less(t, m.TotalAlloc-start, uint64(50000), "must take reasonable amount of memory to recognise the type") }) } } |