Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.23.0
    • Fix Version/s: 2.0.0-alpha
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Harden serialization logic against malformed or malicious input.

      Add range checking to readVInt, to detect overflows, underflows, and larger-than-expected values.

      1. HADOOP-8275.003.patch
        5 kB
        Colin Patrick McCabe
      2. HADOOP-8275.002.patch
        5 kB
        Colin Patrick McCabe
      3. HADOOP-8275.001.patch
        3 kB
        Colin Patrick McCabe

        Issue Links

          Activity

          Hide
          Aaron T. Myers added a comment -

          Hey Colin, could you please add some simple tests for this code? Also, be sure to mark it patch-available so that test-patch will run on it. (I'm doing that for you now.)

          Show
          Aaron T. Myers added a comment - Hey Colin, could you please add some simple tests for this code? Also, be sure to mark it patch-available so that test-patch will run on it. (I'm doing that for you now.)
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12522627/HADOOP-8275.001.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in .

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/851//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/851//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12522627/HADOOP-8275.001.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in . +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/851//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/851//console This message is automatically generated.
          Hide
          Colin Patrick McCabe added a comment -

          Hi atm,

          Thanks for making this runnable by Jenkins. I forgot to do that!

          The tests are actually in the patch for HDFS-3134, which depends on this patch. I guess I should have commented on that earlier...

          Show
          Colin Patrick McCabe added a comment - Hi atm, Thanks for making this runnable by Jenkins. I forgot to do that! The tests are actually in the patch for HDFS-3134 , which depends on this patch. I guess I should have commented on that earlier...
          Hide
          Tom White added a comment -

          Can you write some unit tests for the new behaviour/method in this patch? It looks like the test in HDFS-3134 is only testing indirectly.

          Show
          Tom White added a comment - Can you write some unit tests for the new behaviour/method in this patch? It looks like the test in HDFS-3134 is only testing indirectly.
          Hide
          Colin Patrick McCabe added a comment -
          • add unit test
          Show
          Colin Patrick McCabe added a comment - add unit test
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12522643/HADOOP-8275.002.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 1 new or modified test files.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in .

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/852//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/852//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12522643/HADOOP-8275.002.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 1 new or modified test files. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in . +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/852//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/852//console This message is automatically generated.
          Hide
          Eli Collins added a comment -

          Maybe update the jira subject to "readVInt range checking to restrict DelegateKey len to 1mb" or similar?

          Nits:

          • MAX_KEY_LEN = 1024*1024 would be more readable
          • @return javadocs don't get periods (readVIntInRange)

          For sanity, check the impact on the load time of a large edit log since readVInt is called frequently? Otherwise looks great!

          Show
          Eli Collins added a comment - Maybe update the jira subject to "readVInt range checking to restrict DelegateKey len to 1mb" or similar? Nits: MAX_KEY_LEN = 1024*1024 would be more readable @return javadocs don't get periods (readVIntInRange) For sanity, check the impact on the load time of a large edit log since readVInt is called frequently? Otherwise looks great!
          Hide
          Colin Patrick McCabe added a comment -
          • remove period from javadoc
          • use 1024 * 1024 instead of 1048576
          Show
          Colin Patrick McCabe added a comment - remove period from javadoc use 1024 * 1024 instead of 1048576
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12524955/HADOOP-8275.003.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 1 new or modified test files.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests:
          org.apache.hadoop.fs.viewfs.TestViewFsTrash
          org.apache.hadoop.io.TestSequenceFile

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/900//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/900//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12524955/HADOOP-8275.003.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 1 new or modified test files. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.fs.viewfs.TestViewFsTrash org.apache.hadoop.io.TestSequenceFile +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/900//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/900//console This message is automatically generated.
          Hide
          Colin Patrick McCabe added a comment -

          @eli: I re-ran some of the tests and found no appreciable performance difference (including the oev test, which does a lot of serialization/deserialization).

          Show
          Colin Patrick McCabe added a comment - @eli: I re-ran some of the tests and found no appreciable performance difference (including the oev test, which does a lot of serialization/deserialization).
          Hide
          Eli Collins added a comment -

          +1 looks good. Test failures are unrelated (HADOOP-8330 and HADOOP-8110).

          Show
          Eli Collins added a comment - +1 looks good. Test failures are unrelated ( HADOOP-8330 and HADOOP-8110 ).
          Hide
          Eli Collins added a comment -

          Forgot to mention, jira for other places where we need to use readVInt range checking?

          Show
          Eli Collins added a comment - Forgot to mention, jira for other places where we need to use readVInt range checking?
          Hide
          Eli Collins added a comment -

          I've committed this and merged to branch-2. Thanks Colin!

          Show
          Eli Collins added a comment - I've committed this and merged to branch-2. Thanks Colin!
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #2169 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2169/)
          HADOOP-8275. Range check DelegationKey length. Contributed by Colin Patrick McCabe (Revision 1332839)

          Result = SUCCESS
          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1332839
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/WritableUtils.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/DelegationKey.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/TestWritableUtils.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #2169 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2169/ ) HADOOP-8275 . Range check DelegationKey length. Contributed by Colin Patrick McCabe (Revision 1332839) Result = SUCCESS eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1332839 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/WritableUtils.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/DelegationKey.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/TestWritableUtils.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #2243 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2243/)
          HADOOP-8275. Range check DelegationKey length. Contributed by Colin Patrick McCabe (Revision 1332839)

          Result = SUCCESS
          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1332839
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/WritableUtils.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/DelegationKey.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/TestWritableUtils.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #2243 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2243/ ) HADOOP-8275 . Range check DelegationKey length. Contributed by Colin Patrick McCabe (Revision 1332839) Result = SUCCESS eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1332839 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/WritableUtils.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/DelegationKey.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/TestWritableUtils.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #2185 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2185/)
          HADOOP-8275. Range check DelegationKey length. Contributed by Colin Patrick McCabe (Revision 1332839)

          Result = ABORTED
          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1332839
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/WritableUtils.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/DelegationKey.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/TestWritableUtils.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #2185 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2185/ ) HADOOP-8275 . Range check DelegationKey length. Contributed by Colin Patrick McCabe (Revision 1332839) Result = ABORTED eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1332839 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/WritableUtils.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/DelegationKey.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/TestWritableUtils.java
          Hide
          Colin Patrick McCabe added a comment -

          Forgot to mention, jira for other places where we need to use readVInt range checking?

          I think HDFS-3134 takes care of all the other cases. It also adds a fuzz tester for the edit log.

          I filed HDFS-3346 for FSImage fuzz testing.

          Show
          Colin Patrick McCabe added a comment - Forgot to mention, jira for other places where we need to use readVInt range checking? I think HDFS-3134 takes care of all the other cases. It also adds a fuzz tester for the edit log. I filed HDFS-3346 for FSImage fuzz testing.
          Hide
          Eli Collins added a comment -

          Beautiful

          Show
          Eli Collins added a comment - Beautiful
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #1032 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1032/)
          HADOOP-8275. Range check DelegationKey length. Contributed by Colin Patrick McCabe (Revision 1332839)

          Result = FAILURE
          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1332839
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/WritableUtils.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/DelegationKey.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/TestWritableUtils.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1032 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1032/ ) HADOOP-8275 . Range check DelegationKey length. Contributed by Colin Patrick McCabe (Revision 1332839) Result = FAILURE eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1332839 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/WritableUtils.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/DelegationKey.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/TestWritableUtils.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1067 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1067/)
          HADOOP-8275. Range check DelegationKey length. Contributed by Colin Patrick McCabe (Revision 1332839)

          Result = FAILURE
          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1332839
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/WritableUtils.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/DelegationKey.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/TestWritableUtils.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1067 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1067/ ) HADOOP-8275 . Range check DelegationKey length. Contributed by Colin Patrick McCabe (Revision 1332839) Result = FAILURE eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1332839 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/WritableUtils.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/DelegationKey.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/TestWritableUtils.java

            People

            • Assignee:
              Colin Patrick McCabe
              Reporter:
              Colin Patrick McCabe
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development