Skip to content

Commit 56e0598

Browse files
Martin Bentancourmikecp
authored andcommitted
Validate fileName to prevent path traversal
1 parent c3625ae commit 56e0598

File tree

3 files changed

+41
-29
lines changed

3 files changed

+41
-29
lines changed

src/Umbraco.Web.UI/Umbraco/config/lang/en.xml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -323,6 +323,7 @@
323323
<key alias="orClickHereToUpload">or click here to choose files</key>
324324
<key alias="dragFilesHereToUpload">You can drag files here to upload</key>
325325
<key alias="disallowedFileType">Cannot upload this file, it does not have an approved file type</key>
326+
<key alias="invalidFileName">Cannot upload this file, it does not have a valid file name</key>
326327
<key alias="maxFileSize">Max file size is</key>
327328
<key alias="mediaRoot">Media root</key>
328329
<key alias="moveFailed">Failed to move media</key>

src/Umbraco.Web.UI/Umbraco/config/lang/en_us.xml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -328,6 +328,7 @@
328328
<key alias="orClickHereToUpload">or click here to choose files</key>
329329
<key alias="dragFilesHereToUpload">You can drag files here to upload.</key>
330330
<key alias="disallowedFileType">Cannot upload this file, it does not have an approved file type</key>
331+
<key alias="invalidFileName">Cannot upload this file, it does not have a valid file name</key>
331332
<key alias="maxFileSize">Max file size is</key>
332333
<key alias="mediaRoot">Media root</key>
333334
<key alias="moveFailed">Failed to move media</key>

src/Umbraco.Web/Editors/ContentTypeController.cs

Lines changed: 39 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -576,43 +576,53 @@ public async Task<ContentTypeImportModel> Upload()
576576
var fileName = file.Headers.ContentDisposition.FileName.Trim(Constants.CharArrays.DoubleQuote);
577577
var ext = fileName.Substring(fileName.LastIndexOf('.') + 1).ToLower();
578578

579-
var destFileName = root + "\\" + fileName;
580-
try
581-
{
582-
// due to a bug before 8.7.0 we didn't delete temp files, so we need to make sure to delete before
583-
// moving else you get errors and the upload fails without a message in the UI (there's a JS error)
584-
if(System.IO.File.Exists(destFileName))
585-
System.IO.File.Delete(destFileName);
586-
587-
// renaming the file because MultipartFormDataStreamProvider has created a random fileName instead of using the name from the
588-
// content-disposition for more than 6 years now. Creating a CustomMultipartDataStreamProvider deriving from MultipartFormDataStreamProvider
589-
// seems like a cleaner option, but I'm not sure where to put it and renaming only takes one line of code.
590-
System.IO.File.Move(result.FileData[0].LocalFileName, destFileName);
591-
}
592-
catch (Exception ex)
579+
var destFileName = Path.Combine(root, fileName);
580+
if (Path.GetFullPath(destFileName).StartsWith(Path.GetFullPath(root)))
593581
{
594-
Logger.Error<ContentTypeController, string>(ex, "Error uploading udt file to App_Data: {File}", destFileName);
595-
}
596-
597-
if (ext.InvariantEquals("udt"))
598-
{
599-
model.TempFileName = Path.Combine(root, fileName);
582+
try
583+
{
584+
// due to a bug before 8.7.0 we didn't delete temp files, so we need to make sure to delete before
585+
// moving else you get errors and the upload fails without a message in the UI (there's a JS error)
586+
if(System.IO.File.Exists(destFileName))
587+
System.IO.File.Delete(destFileName);
588+
589+
// renaming the file because MultipartFormDataStreamProvider has created a random fileName instead of using the name from the
590+
// content-disposition for more than 6 years now. Creating a CustomMultipartDataStreamProvider deriving from MultipartFormDataStreamProvider
591+
// seems like a cleaner option, but I'm not sure where to put it and renaming only takes one line of code.
592+
System.IO.File.Move(result.FileData[0].LocalFileName, destFileName);
593+
}
594+
catch (Exception ex)
595+
{
596+
Logger.Error<ContentTypeController, string>(ex, "Error uploading udt file to App_Data: {File}", destFileName);
597+
}
600598

601-
var xd = new XmlDocument
599+
if (ext.InvariantEquals("udt"))
602600
{
603-
XmlResolver = null
604-
};
605-
xd.Load(model.TempFileName);
601+
model.TempFileName = destFileName;
602+
603+
var xd = new XmlDocument
604+
{
605+
XmlResolver = null
606+
};
607+
xd.Load(model.TempFileName);
606608

607-
model.Alias = xd.DocumentElement?.SelectSingleNode("//DocumentType/Info/Alias")?.FirstChild.Value;
608-
model.Name = xd.DocumentElement?.SelectSingleNode("//DocumentType/Info/Name")?.FirstChild.Value;
609+
model.Alias = xd.DocumentElement?.SelectSingleNode("//DocumentType/Info/Alias")?.FirstChild.Value;
610+
model.Name = xd.DocumentElement?.SelectSingleNode("//DocumentType/Info/Name")?.FirstChild.Value;
611+
}
612+
else
613+
{
614+
model.Notifications.Add(new Notification(
615+
Services.TextService.Localize("speechBubbles", "operationFailedHeader"),
616+
Services.TextService.Localize("media", "disallowedFileType"),
617+
NotificationStyle.Warning));
618+
}
609619
}
610620
else
611621
{
612622
model.Notifications.Add(new Notification(
613-
Services.TextService.Localize("speechBubbles", "operationFailedHeader"),
614-
Services.TextService.Localize("media", "disallowedFileType"),
615-
NotificationStyle.Warning));
623+
Services.TextService.Localize("speechBubbles", "operationFailedHeader"),
624+
Services.TextService.Localize("media", "invalidFileName"),
625+
NotificationStyle.Warning));
616626
}
617627

618628
return model;

0 commit comments

Comments
 (0)