Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 4.2.0
    • Fix Version/s: 4.3.0
    • Component/s: core, security
    • Labels:

      Description

      In method inClause of org.apache.oozie.executor.jpa.BulkJPAExecutor there is a poosibility for SQL injection (https://www.owasp.org/index.php/SQL_injection) : there is no validation of content of string name before it's included in sql script, opening a possibility for a malicious user to inject sql commands.
      A simple validation of strings using .matches(...) would fix problem.

      1. 0001-OOZIE-2362-SQL-injection-in-BulkJPAExecutor.patch
        2 kB
        thierry accart
      2. OOZIE-2362-001.patch
        10 kB
        Peter Bacsko
      3. OOZIE-2362-002.patch
        10 kB
        Peter Bacsko

        Activity

        Hide
        rkanter Robert Kanter added a comment -

        Closing issue; Oozie 4.3.0 is released.

        Show
        rkanter Robert Kanter added a comment - Closing issue; Oozie 4.3.0 is released.
        Hide
        rkanter Robert Kanter added a comment -

        Thanks Peter. Committed to master!

        Show
        rkanter Robert Kanter added a comment - Thanks Peter. Committed to master!
        Hide
        rkanter Robert Kanter added a comment -

        I'll fix the long lines on committing, but please take care of that next time.

        +1

        Show
        rkanter Robert Kanter added a comment - I'll fix the long lines on committing, but please take care of that next time. +1
        Hide
        hadoopqa Hadoop QA added a comment -

        Testing JIRA OOZIE-2362

        Cleaning local git workspace

        ----------------------------

        +1 PATCH_APPLIES
        +1 CLEAN
        -1 RAW_PATCH_ANALYSIS
        . +1 the patch does not introduce any @author tags
        . +1 the patch does not introduce any tabs
        . +1 the patch does not introduce any trailing spaces
        . -1 the patch contains 2 line(s) longer than 132 characters
        . -1 the patch does not add/modify any testcase
        +1 RAT
        . +1 the patch does not seem to introduce new RAT warnings
        +1 JAVADOC
        . +1 the patch does not seem to introduce new Javadoc warnings
        +1 COMPILE
        . +1 HEAD compiles
        . +1 patch compiles
        . +1 the patch does not seem to introduce new javac warnings
        +1 BACKWARDS_COMPATIBILITY
        . +1 the patch does not change any JPA Entity/Colum/Basic/Lob/Transient annotations
        . +1 the patch does not modify JPA files
        -1 TESTS
        . Tests run: 1787
        . Tests failed: 1
        . Tests errors: 0

        . The patch failed the following testcases:

        . testBundleStatusTransitWithLock(org.apache.oozie.service.TestStatusTransitService)

        +1 DISTRO
        . +1 distro tarball builds with the patch

        ----------------------------
        -1 Overall result, please check the reported -1(s)

        The full output of the test-patch run is available at

        . https://builds.apache.org/job/oozie-trunk-precommit-build/3002/

        Show
        hadoopqa Hadoop QA added a comment - Testing JIRA OOZIE-2362 Cleaning local git workspace ---------------------------- +1 PATCH_APPLIES +1 CLEAN -1 RAW_PATCH_ANALYSIS . +1 the patch does not introduce any @author tags . +1 the patch does not introduce any tabs . +1 the patch does not introduce any trailing spaces . -1 the patch contains 2 line(s) longer than 132 characters . -1 the patch does not add/modify any testcase +1 RAT . +1 the patch does not seem to introduce new RAT warnings +1 JAVADOC . +1 the patch does not seem to introduce new Javadoc warnings +1 COMPILE . +1 HEAD compiles . +1 patch compiles . +1 the patch does not seem to introduce new javac warnings +1 BACKWARDS_COMPATIBILITY . +1 the patch does not change any JPA Entity/Colum/Basic/Lob/Transient annotations . +1 the patch does not modify JPA files -1 TESTS . Tests run: 1787 . Tests failed: 1 . Tests errors: 0 . The patch failed the following testcases: . testBundleStatusTransitWithLock(org.apache.oozie.service.TestStatusTransitService) +1 DISTRO . +1 distro tarball builds with the patch ---------------------------- -1 Overall result, please check the reported -1(s) The full output of the test-patch run is available at . https://builds.apache.org/job/oozie-trunk-precommit-build/3002/
        Hide
        rkanter Robert Kanter added a comment -

        Looks good; here's a few comments:

        1. Let's avoid using Guava unless necessary. They have a tendency to remove methods or be incompatible.
          List<String> params = Lists.newArrayList();
          

          can just be

          List<String> params = new ArrayList<String>();
          
        2. There seems to be a few places where we're getting bulkFilter.get(BulkResponseImpl.BULK_FILTER_STATUS). Can we get that once and pass it around like we're doing with bulkFilter.get(BulkResponseImpl.BULK_FILTER_COORD)?
        Show
        rkanter Robert Kanter added a comment - Looks good; here's a few comments: Let's avoid using Guava unless necessary. They have a tendency to remove methods or be incompatible. List< String > params = Lists.newArrayList(); can just be List< String > params = new ArrayList< String >(); There seems to be a few places where we're getting bulkFilter.get(BulkResponseImpl.BULK_FILTER_STATUS) . Can we get that once and pass it around like we're doing with bulkFilter.get(BulkResponseImpl.BULK_FILTER_COORD) ?
        Hide
        hadoopqa Hadoop QA added a comment -

        Testing JIRA OOZIE-2362

        Cleaning local git workspace

        ----------------------------

        +1 PATCH_APPLIES
        +1 CLEAN
        -1 RAW_PATCH_ANALYSIS
        . +1 the patch does not introduce any @author tags
        . +1 the patch does not introduce any tabs
        . +1 the patch does not introduce any trailing spaces
        . -1 the patch contains 2 line(s) longer than 132 characters
        . -1 the patch does not add/modify any testcase
        +1 RAT
        . +1 the patch does not seem to introduce new RAT warnings
        +1 JAVADOC
        . +1 the patch does not seem to introduce new Javadoc warnings
        +1 COMPILE
        . +1 HEAD compiles
        . +1 patch compiles
        . +1 the patch does not seem to introduce new javac warnings
        +1 BACKWARDS_COMPATIBILITY
        . +1 the patch does not change any JPA Entity/Colum/Basic/Lob/Transient annotations
        . +1 the patch does not modify JPA files
        -1 TESTS
        . Tests run: 1787
        . Tests failed: 2
        . Tests errors: 0

        . The patch failed the following testcases:

        . testMaxMatThrottleNotPicked(org.apache.oozie.service.TestCoordMaterializeTriggerService)
        . testBundleStatusTransitWithLock(org.apache.oozie.service.TestStatusTransitService)

        +1 DISTRO
        . +1 distro tarball builds with the patch

        ----------------------------
        -1 Overall result, please check the reported -1(s)

        The full output of the test-patch run is available at

        . https://builds.apache.org/job/oozie-trunk-precommit-build/2995/

        Show
        hadoopqa Hadoop QA added a comment - Testing JIRA OOZIE-2362 Cleaning local git workspace ---------------------------- +1 PATCH_APPLIES +1 CLEAN -1 RAW_PATCH_ANALYSIS . +1 the patch does not introduce any @author tags . +1 the patch does not introduce any tabs . +1 the patch does not introduce any trailing spaces . -1 the patch contains 2 line(s) longer than 132 characters . -1 the patch does not add/modify any testcase +1 RAT . +1 the patch does not seem to introduce new RAT warnings +1 JAVADOC . +1 the patch does not seem to introduce new Javadoc warnings +1 COMPILE . +1 HEAD compiles . +1 patch compiles . +1 the patch does not seem to introduce new javac warnings +1 BACKWARDS_COMPATIBILITY . +1 the patch does not change any JPA Entity/Colum/Basic/Lob/Transient annotations . +1 the patch does not modify JPA files -1 TESTS . Tests run: 1787 . Tests failed: 2 . Tests errors: 0 . The patch failed the following testcases: . testMaxMatThrottleNotPicked(org.apache.oozie.service.TestCoordMaterializeTriggerService) . testBundleStatusTransitWithLock(org.apache.oozie.service.TestStatusTransitService) +1 DISTRO . +1 distro tarball builds with the patch ---------------------------- -1 Overall result, please check the reported -1(s) The full output of the test-patch run is available at . https://builds.apache.org/job/oozie-trunk-precommit-build/2995/
        Hide
        pbacsko Peter Bacsko added a comment - - edited

        Examples:

        *** Action query: SELECT a.id, a.actionNumber, a.errorCode, a.errorMessage, a.externalId, a.externalStatus, a.statusStr, a.createdTimestamp, a.nominalTimestamp, a.missingDependencies, c.id, c.appName, c.statusStr FROM CoordinatorActionBean a, CoordinatorJobBean c WHERE a.jobId = c.id AND c.bundleId = :bundleId AND a.nominalTimestamp <= :endNominal AND a.nominalTimestamp >= :startNominal AND a.createdTimestamp <= :endCreated AND a.createdTimestamp >= :startCreated AND a.statusStr IN ('FAILED','KILLED')  ORDER BY a.jobId, a.createdTimestamp
        *** Count query: SELECT COUNT(a) FROM CoordinatorActionBean a, CoordinatorJobBean c WHERE a.jobId = c.id  AND c.bundleId IN ('0000000-160615161217850-oozie-pbac-B')  AND a.nominalTimestamp <= :endNominal AND a.nominalTimestamp >= :startNominal AND a.createdTimestamp <= :endCreated AND a.createdTimestamp >= :startCreated AND a.statusStr IN ('FAILED','KILLED')  
        

        vs

        *** Action query: SELECT a.id, a.actionNumber, a.errorCode, a.errorMessage, a.externalId, a.externalStatus, a.statusStr, a.createdTimestamp, a.nominalTimestamp, a.missingDependencies, c.id, c.appName, c.statusStr FROM CoordinatorActionBean a, CoordinatorJobBean c WHERE a.jobId = c.id AND c.bundleId = :bundleId AND a.nominalTimestamp <= :endNominal AND a.nominalTimestamp >= :startNominal AND a.createdTimestamp <= :endCreated AND a.createdTimestamp >= :startCreated AND a.statusStr IN (:status0, :status1)  ORDER BY a.jobId, a.createdTimestamp
        *** count query: SELECT COUNT(a) FROM CoordinatorActionBean a, CoordinatorJobBean c WHERE a.jobId = c.id  AND c.bundleId IN (:count0)  AND a.nominalTimestamp <= :endNominal AND a.nominalTimestamp >= :startNominal AND a.createdTimestamp <= :endCreated AND a.createdTimestamp >= :startCreated AND a.statusStr IN (:status0, :status1)  
        Param set - count0: 0000000-160615160302963-oozie-pbac-B
        Param set - status0: FAILED
        Param set - status1: KILLED
        
        Show
        pbacsko Peter Bacsko added a comment - - edited Examples: *** Action query: SELECT a.id, a.actionNumber, a.errorCode, a.errorMessage, a.externalId, a.externalStatus, a.statusStr, a.createdTimestamp, a.nominalTimestamp, a.missingDependencies, c.id, c.appName, c.statusStr FROM CoordinatorActionBean a, CoordinatorJobBean c WHERE a.jobId = c.id AND c.bundleId = :bundleId AND a.nominalTimestamp <= :endNominal AND a.nominalTimestamp >= :startNominal AND a.createdTimestamp <= :endCreated AND a.createdTimestamp >= :startCreated AND a.statusStr IN ('FAILED','KILLED') ORDER BY a.jobId, a.createdTimestamp *** Count query: SELECT COUNT(a) FROM CoordinatorActionBean a, CoordinatorJobBean c WHERE a.jobId = c.id AND c.bundleId IN ('0000000-160615161217850-oozie-pbac-B') AND a.nominalTimestamp <= :endNominal AND a.nominalTimestamp >= :startNominal AND a.createdTimestamp <= :endCreated AND a.createdTimestamp >= :startCreated AND a.statusStr IN ('FAILED','KILLED') vs *** Action query: SELECT a.id, a.actionNumber, a.errorCode, a.errorMessage, a.externalId, a.externalStatus, a.statusStr, a.createdTimestamp, a.nominalTimestamp, a.missingDependencies, c.id, c.appName, c.statusStr FROM CoordinatorActionBean a, CoordinatorJobBean c WHERE a.jobId = c.id AND c.bundleId = :bundleId AND a.nominalTimestamp <= :endNominal AND a.nominalTimestamp >= :startNominal AND a.createdTimestamp <= :endCreated AND a.createdTimestamp >= :startCreated AND a.statusStr IN (:status0, :status1) ORDER BY a.jobId, a.createdTimestamp *** count query: SELECT COUNT(a) FROM CoordinatorActionBean a, CoordinatorJobBean c WHERE a.jobId = c.id AND c.bundleId IN (:count0) AND a.nominalTimestamp <= :endNominal AND a.nominalTimestamp >= :startNominal AND a.createdTimestamp <= :endCreated AND a.createdTimestamp >= :startCreated AND a.statusStr IN (:status0, :status1) Param set - count0: 0000000-160615160302963-oozie-pbac-B Param set - status0: FAILED Param set - status1: KILLED
        Hide
        pbacsko Peter Bacsko added a comment -

        I assigned this to myself. I'm soon going to provide a patch where the query is generated with named parameters.

        Show
        pbacsko Peter Bacsko added a comment - I assigned this to myself. I'm soon going to provide a patch where the query is generated with named parameters.
        Hide
        taccart thierry accart added a comment -

        IMHO, the correct fix shall not build a sql query based on parameters : the patch is a quick fix.
        To make something correct, code shall never build sql query but should check parameters, reject any request with incorrect parameters, or, if all parameters are correct, use prepared statements (for example).

        Show
        taccart thierry accart added a comment - IMHO, the correct fix shall not build a sql query based on parameters : the patch is a quick fix. To make something correct, code shall never build sql query but should check parameters, reject any request with incorrect parameters, or, if all parameters are correct, use prepared statements (for example).
        Hide
        jaydeepvishwakarma Jaydeep Vishwakarma added a comment -

        Is there specific reason to replace special characters with "-"? Will it not be completely different query? Can we replace with empty character.

        Show
        jaydeepvishwakarma Jaydeep Vishwakarma added a comment - Is there specific reason to replace special characters with "-"? Will it not be completely different query? Can we replace with empty character.
        Hide
        jaydeepvishwakarma Jaydeep Vishwakarma added a comment -

        Is there specific reason to replace special characters with "-"? Will it not be completely different query? Can we replace with empty character.

        Show
        jaydeepvishwakarma Jaydeep Vishwakarma added a comment - Is there specific reason to replace special characters with "-"? Will it not be completely different query? Can we replace with empty character.
        Hide
        hadoopqa Hadoop QA added a comment -

        Testing JIRA OOZIE-2362

        Cleaning local git workspace

        ----------------------------

        -1 Patch failed to apply to head of branch

        ----------------------------

        Show
        hadoopqa Hadoop QA added a comment - Testing JIRA OOZIE-2362 Cleaning local git workspace ---------------------------- -1 Patch failed to apply to head of branch ----------------------------
        Hide
        hadoopqa Hadoop QA added a comment -

        Testing JIRA OOZIE-2362

        Cleaning local git workspace

        ----------------------------

        -1 Patch failed to apply to head of branch

        ----------------------------

        Show
        hadoopqa Hadoop QA added a comment - Testing JIRA OOZIE-2362 Cleaning local git workspace ---------------------------- -1 Patch failed to apply to head of branch ----------------------------
        Hide
        taccart thierry accart added a comment -

        Patch for BulkJPAExecutor.java

        Show
        taccart thierry accart added a comment - Patch for BulkJPAExecutor.java
        Hide
        taccart thierry accart added a comment -

        Replace any non word (a-zA-Z_0-9) character by a minus sign using name.replaceAll("\\W","-") instead of name in method inClause

        if (firstVal)

        { sb.append(" AND " + type + "." + col + " IN (\'" + name.replaceAll( "[\\W]","-") + "\'"); firstVal = false; }

        else

        { sb.append(",\'" + name.replaceAll( "[\\W]","-") + "\'"); }
        Show
        taccart thierry accart added a comment - Replace any non word (a-zA-Z_0-9) character by a minus sign using name.replaceAll(" \\W ","-") instead of name in method inClause if (firstVal) { sb.append(" AND " + type + "." + col + " IN (\'" + name.replaceAll( "[\\W]","-") + "\'"); firstVal = false; } else { sb.append(",\'" + name.replaceAll( "[\\W]","-") + "\'"); }

          People

          • Assignee:
            pbacsko Peter Bacsko
            Reporter:
            taccart thierry accart
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development