You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ofbiz.apache.org by jl...@apache.org on 2019/08/10 15:51:19 UTC
svn commit: r1864881 -
/ofbiz/ofbiz-framework/trunk/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/FrameImage.java
Author: jleroux
Date: Sat Aug 10 15:51:19 2019
New Revision: 1864881
URL: http://svn.apache.org/viewvc?rev=1864881&view=rev
Log:
Fixed: [FB] Find Security Bugs
(OFBIZ-9973)
This fixes an issue in FrameImage::uploadFrame which was reported by Man Yue Mo
as described in OFBIZ-9973
I finally decided to follow OWASP advice about using normalize()
Modified:
ofbiz/ofbiz-framework/trunk/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/FrameImage.java
Modified: ofbiz/ofbiz-framework/trunk/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/FrameImage.java
URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/FrameImage.java?rev=1864881&r1=1864880&r2=1864881&view=diff
==============================================================================
--- ofbiz/ofbiz-framework/trunk/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/FrameImage.java (original)
+++ ofbiz/ofbiz-framework/trunk/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/FrameImage.java Sat Aug 10 15:51:19 2019
@@ -30,7 +30,6 @@ import java.awt.image.RenderedImage;
import java.io.File;
import java.io.IOException;
import java.io.RandomAccessFile;
-import java.net.URLEncoder;
import java.nio.ByteBuffer;
import java.util.HashMap;
import java.util.Locale;
@@ -291,7 +290,7 @@ public class FrameImage {
String dataResourceId = null;
try {
String dirPath = "/frame/";
- File dir = new File(imageServerPath + dirPath);
+ File dir = new File(imageServerPath + dirPath).toPath().normalize().toFile(); // cf. OFBIZ-9973
if (!dir.exists()) {
boolean createDir = dir.mkdir();
if (!createDir) {
@@ -300,7 +299,7 @@ public class FrameImage {
}
}
String imagePath = "/frame/" + imageName;
- File file = new File(imageServerPath + imagePath);
+ File file = new File(imageServerPath + imagePath).toPath().normalize().toFile(); // cf. OFBIZ-9973
if (file.exists()) {
request.setAttribute("_ERROR_MESSAGE_", "There is an existing frame, please select from the existing frame.");
return "error";
@@ -399,7 +398,7 @@ public class FrameImage {
Debug.logError("File :" + file.getName() + ", couldn't be loaded", module);
}
// Image Frame
- BufferedImage bufImg1 = ImageIO.read(new File(URLEncoder.encode(imageServerPath + "/" + productId + "/" + imageName, "UTF-8")).getCanonicalFile());
+ BufferedImage bufImg1 = ImageIO.read(new File(imageServerPath + "/" + productId + "/" + imageName).toPath().normalize().toFile()); // cf. OFBIZ-9973
BufferedImage bufImg2 = ImageIO.read(new File(imageServerPath + "/frame/" + frameImageName));
int bufImgType;
@@ -457,7 +456,7 @@ public class FrameImage {
public static String deleteFrameImage(HttpServletRequest request, HttpServletResponse response) throws IOException {
Map<String, ? extends Object> context = UtilGenerics.cast(request.getParameterMap());
String imageServerPath = FlexibleStringExpander.expandString(EntityUtilProperties.getPropertyValue("catalog", "image.management.path", (Delegator) context.get("delegator")), context);
- File file = new File(imageServerPath + "/preview/" + "/previewImage.jpg").getCanonicalFile();
+ File file = new File(imageServerPath + "/preview/" + "/previewImage.jpg");
if (file.exists()) {
if (!file.delete()) {
Debug.logError("File :" + file.getName() + ", couldn't be deleted", module);
Re: svn commit: r1864881 - /ofbiz/ofbiz-framework/trunk/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/FrameImage.java
Posted by Mathieu Lirzin <ma...@nereide.fr>.
Jacques Le Roux <ja...@les7arts.com> writes:
> Done
Thanks Jacques.
--
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761 070D 0ADE E100 9460 4D37
Re: svn commit: r1864881 -
/ofbiz/ofbiz-framework/trunk/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/FrameImage.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
Done
Le 10/08/2019 à 19:29, Jacques Le Roux a écrit :
> Makes sense Mathieu,
>
> I'll...
>
> Jacques
>
> Le 10/08/2019 à 18:43, Mathieu Lirzin a écrit :
>> Hello Jacques,
>>
>> jleroux@apache.org writes:
>>
>>> - File dir = new File(imageServerPath + dirPath);
>>> + File dir = new File(imageServerPath + dirPath).toPath().normalize().toFile(); // cf. OFBIZ-9973
>> IMO This comment should include a short rationale too. Referencing a
>> bug report is really useful when the issue is complex and the reader
>> might want to understand the details leading to a non-obvious piece of
>> code, but it would be better to *not* require the developers to do the
>> extra work of looking at JIRA to get a broad idea of the rationale. I
>> would recommend the something like:
>>
>> --8<---------------cut here---------------start------------->8---
>> // Normalize the path because we want to ensure that ... (see OFBIZ-9973 for more details).
>> --8<---------------cut here---------------end--------------->8---
>>
>> Additionally in order to avoid repeating such comment in multiple places
>> creating a helper method factorizing the code related to fixing that
>> issue and putting the comment ontop of the method definition is nice
>> too.
>>
>> My 2¢.
>>
>> Thanks Jacques.
>>
>
Re: svn commit: r1864881 -
/ofbiz/ofbiz-framework/trunk/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/FrameImage.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
Makes sense Mathieu,
I'll...
Jacques
Le 10/08/2019 à 18:43, Mathieu Lirzin a écrit :
> Hello Jacques,
>
> jleroux@apache.org writes:
>
>> - File dir = new File(imageServerPath + dirPath);
>> + File dir = new File(imageServerPath + dirPath).toPath().normalize().toFile(); // cf. OFBIZ-9973
> IMO This comment should include a short rationale too. Referencing a
> bug report is really useful when the issue is complex and the reader
> might want to understand the details leading to a non-obvious piece of
> code, but it would be better to *not* require the developers to do the
> extra work of looking at JIRA to get a broad idea of the rationale. I
> would recommend the something like:
>
> --8<---------------cut here---------------start------------->8---
> // Normalize the path because we want to ensure that ... (see OFBIZ-9973 for more details).
> --8<---------------cut here---------------end--------------->8---
>
> Additionally in order to avoid repeating such comment in multiple places
> creating a helper method factorizing the code related to fixing that
> issue and putting the comment ontop of the method definition is nice
> too.
>
> My 2¢.
>
> Thanks Jacques.
>
Re: svn commit: r1864881 - /ofbiz/ofbiz-framework/trunk/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/FrameImage.java
Posted by Mathieu Lirzin <ma...@nereide.fr>.
Hello Jacques,
jleroux@apache.org writes:
> - File dir = new File(imageServerPath + dirPath);
> + File dir = new File(imageServerPath + dirPath).toPath().normalize().toFile(); // cf. OFBIZ-9973
IMO This comment should include a short rationale too. Referencing a
bug report is really useful when the issue is complex and the reader
might want to understand the details leading to a non-obvious piece of
code, but it would be better to *not* require the developers to do the
extra work of looking at JIRA to get a broad idea of the rationale. I
would recommend the something like:
--8<---------------cut here---------------start------------->8---
// Normalize the path because we want to ensure that ... (see OFBIZ-9973 for more details).
--8<---------------cut here---------------end--------------->8---
Additionally in order to avoid repeating such comment in multiple places
creating a helper method factorizing the code related to fixing that
issue and putting the comment ontop of the method definition is nice
too.
My 2¢.
Thanks Jacques.
--
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761 070D 0ADE E100 9460 4D37