[jira] [Commented] (OFBIZ-9973) [FB] Find Security Bugs

[Nicolas Malin (Jira)]

    [ https://issues.apache.org/jira/browse/OFBIZ-9973?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16904463#comment-16904463 ]

Jacques Le Roux commented on OFBIZ-9973:
----------------------------------------

Almost a year ago we received this security report by Man Yue Mo  from Semmle:

{quote}
From: "Man Yue Mo via RT" <[hidden email]>
To: [hidden email]
Subject: [Semmle Security Reports #17190] Path traversal by authenticated users
Date: 2018/09/24 15:05:43
Private: YES
List: [hidden email]

Hi,

There are two path traversal issues by authenticated users. To reproduce these issues from a fresh install, first upload an image via the url

https://localhost:8443/catalog/control/ImageUpload

to ensure that the directory `/framework/images/webapp/products/management` exists.

1. The first one allows arbitrary file write, which is effectively remote code execution as the user can write any jsp or ftl files and get them executed by fetching them again. In the `uploadFrame` method of `FrameImage` class, a file is created here:

https://github.com/apache/ofbiz/blob/release16.11/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/FrameImage.java#L293

The local variable `imageName` used in `imagePath` is taken from the `imageFileName` component of the map `tempFile`:

https://github.com/apache/ofbiz/blob/release16.11/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/FrameImage.java#L264

which comes from the `name` property of an `FileItem`:

https://github.com/apache/ofbiz/blob/release16.11/applications/content/src/main/java/org/apache/ofbiz/content/layout/LayoutWorker.java#L106

This `name` property is taken from the multi-part request as the `filename` attribute and is in complete control of a remote user.

For example, run the attached `PoC_path.py` will create a `abc.jsp` file in the root directory of the `ofbiz` repo with `abc` inside it. (`ofbiz/abc.jsp` when running an instance built from code pulled from github.) This, however, does not allow overriding of files.

2. In the `previewFrameImage` method of the same class, `productId` is taken from request query parameter:

https://github.com/apache/ofbiz/blob/release16.11/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/FrameImage.java#L340

This is then used to construct a path to fetch an image:

https://github.com/apache/ofbiz/blob/release16.11/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/FrameImage.java#L391

I believe this was also noticed here:

https://jira.apache.org/jira/browse/OFBIZ-9973

However, the conclusion of the ticket is incorrect. First, although `imageName` is encoded, `productId` is not so it is still possible to traverse path. Second, `getCanonicalFile` has nothing to do with preventing path traversal, all it does is to resolve the path and get rid of the `../` and `./` in the path.

This can be tested, for example, by putting an image in the directory (image.png) containing the ofbiz repository, and use the following url:

https://localhost:8443/catalog/control/previewFrameImage?productId=..%2F..%2F..%2F..%2F..%2F..%2F..%2F&frameContentId=10000&frameDataResourceId=10000&imageWidth=-1&imageHeight=-1&imageName=image.png

Here `frameContentId` and `frameDataResourceId` have to be valid and corresponds to an image, from some trial and error, it seems like these ids starts from 10000 and increment when new resource is uploaded, so 10000 has a good chance of working. If successful, you should see an the target image image.png overlay with the image corresponding to the one with `frameContentId` 10000. This will only allow arbitrary reads of image files.

As a fresh instalment of ofbiz only comes with the admin user, I don't know how much privilege is required to access these endpoints or whether this component is just a sample app.

These are tested on the branch release16.11 (commit 4a0f061). Thank you very much for your help and please let me know if there is anything that I can help. Thanks.

Best Regards,

Man Yue Mo
{quote}

As both need an authenticated user they were rejected as not needing a CVE. Nevertheless, Man Yue Mo gave us interesting information. Following Man Yue Mo indication I fixed the 1st issue in
Trunk r1864881
R18 r1864882
R17 r1864883
R16 r1864884


Spotbug still reports a security bug in FrameImage.::previewFrameImage. But still only in trunk, which is weird. Here is the XML content of the report (found in My Eclipse workspace)


{code:xml}
  <BugInstance type="PT_RELATIVE_PATH_TRAVERSAL" priority="2" rank="7" abbrev="PT" category="SECURITY" first="5">
    <Class classname="org.apache.ofbiz.product.imagemanagement.FrameImage">
      <SourceLine classname="org.apache.ofbiz.product.imagemanagement.FrameImage" sourcefile="FrameImage.java" sourcepath="org/apache/ofbiz/product/imagemanagement/FrameImage.java"/>
    </Class>
    <Method classname="org.apache.ofbiz.product.imagemanagement.FrameImage" name="previewFrameImage" signature="(Ljavax/servlet/http/HttpServletRequest;Ljavax/servlet/http/HttpServletResponse;)Ljava/lang/String;" isStatic="true">
      <SourceLine classname="org.apache.ofbiz.product.imagemanagement.FrameImage" start="354" end="427" startBytecode="0" endBytecode="1457" sourcefile="FrameImage.java" sourcepath="org/apache/ofbiz/product/imagemanagement/FrameImage.java"/>
    </Method>
    <Method classname="java.io.File" name="&lt;init&gt;" signature="(Ljava/lang/String;)V" isStatic="false" role="METHOD_CALLED">
      <SourceLine classname="java.io.File" start="275" end="281" startBytecode="0" endBytecode="115" sourcefile="File.java" sourcepath="java/io/File.java"/>
    </Method>
    <String value="imageName" role="STRING_PARAMETER_NAME"/>
    <SourceLine classname="org.apache.ofbiz.product.imagemanagement.FrameImage" start="360" end="360" startBytecode="61" endBytecode="61" sourcefile="FrameImage.java" sourcepath="org/apache/ofbiz/product/imagemanagement/FrameImage.java" role="SOURCE_LINE_GENERATED_AT"/>
    <SourceLine classname="org.apache.ofbiz.product.imagemanagement.FrameImage" start="402" end="402" startBytecode="490" endBytecode="490" sourcefile="FrameImage.java" sourcepath="org/apache/ofbiz/product/imagemanagement/FrameImage.java"/>
    <SourceLine classname="org.apache.ofbiz.product.imagemanagement.FrameImage" start="402" end="402" startBytecode="490" endBytecode="490" sourcefile="FrameImage.java" sourcepath="org/apache/ofbiz/product/imagemanagement/FrameImage.java"/>
  </BugInstance>
  <BugInstance type="PT_RELATIVE_PATH_TRAVERSAL" priority="2" rank="7" abbrev="PT" category="SECURITY" first="1" last="4" removedByChange="true">
    <Class classname="org.apache.ofbiz.product.imagemanagement.FrameImage">
      <SourceLine classname="org.apache.ofbiz.product.imagemanagement.FrameImage" sourcefile="FrameImage.java" sourcepath="org/apache/ofbiz/product/imagemanagement/FrameImage.java"/>
    </Class>
    <Method classname="org.apache.ofbiz.product.imagemanagement.FrameImage" name="previewFrameImage" signature="(Ljavax/servlet/http/HttpServletRequest;Ljavax/servlet/http/HttpServletResponse;)Ljava/lang/String;" isStatic="true">
      <SourceLine classname="org.apache.ofbiz.product.imagemanagement.FrameImage" start="355" end="435" startBytecode="0" endBytecode="1496" sourcefile="FrameImage.java" sourcepath="org/apache/ofbiz/product/imagemanagement/FrameImage.java"/>
    </Method>
    <Method classname="java.io.File" name="&lt;init&gt;" signature="(Ljava/lang/String;)V" isStatic="false" role="METHOD_CALLED">
      <SourceLine classname="java.io.File" start="275" end="281" startBytecode="0" endBytecode="115" sourcefile="File.java" sourcepath="java/io/File.java"/>
    </Method>
    <String value="productId" role="STRING_PARAMETER_NAME"/>
    <SourceLine classname="org.apache.ofbiz.product.imagemanagement.FrameImage" start="360" end="360" startBytecode="48" endBytecode="48" sourcefile="FrameImage.java" sourcepath="org/apache/ofbiz/product/imagemanagement/FrameImage.java" role="SOURCE_LINE_GENERATED_AT"/>
    <SourceLine classname="org.apache.ofbiz.product.imagemanagement.FrameImage" start="410" end="410" startBytecode="512" endBytecode="512" sourcefile="FrameImage.java" sourcepath="org/apache/ofbiz/product/imagemanagement/FrameImage.java"/>
    <SourceLine classname="org.apache.ofbiz.product.imagemanagement.FrameImage" start="410" end="410" startBytecode="512" endBytecode="512" sourcefile="FrameImage.java" sourcepath="org/apache/ofbiz/product/imagemanagement/FrameImage.java"/>
  </BugInstance>
{code}

Obviously it does not take into account the use of normalize(). I have checked and both issues are fixed by using [Path::normalize|https://docs.oracle.com/javase/8/docs/api/java/nio/file/Path.html#normalize--] as adviced by [OWASP in another way|https://www.owasp.org/index.php/File_System#Path_traversal]:
bq. If forced to use user input for file operations, normalize the input before using in file io API's, such as http://docs.oracle.com/javase/7/docs/api/java/net/URI.html#normalize().

> [FB] Find Security Bugs
> -----------------------
>
>                 Key: OFBIZ-9973
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-9973
>             Project: OFBiz
>          Issue Type: Sub-task
>          Components: marketing, product
>    Affects Versions: Trunk, Release Branch 16.11
>            Reporter: Jacques Le Roux
>            Assignee: Jacques Le Roux
>            Priority: Major
>             Fix For: 17.12.01, 16.11.06, 18.12.01
>
>
> I recently [found|https://www.ysofters.com/2015/08/31/taint-analysis-added-to-findbugs/] FindBugs embeds an option [to Find Security Bugs|https://find-sec-bugs.github.io/]:
> I have tried this option: https://github.com/find-sec-bugs/find-sec-bugs/wiki/Eclipse-Tutorial
> Also later we should remember of OFBIZ-7963 and if possible run this tool in [Builbot using Gradle|https://search.maven.org/#search|gav|1|g:%22com.h3xstream.findsecbugs%22%20AND%20a:%22findsecbugs-plugin%22] (did not check feasibility)

--
This message was sent by Atlassian JIRA
(v7.6.14#76016)