JDK-8347377 : Add validation checks for ICC_Profile header fields #23044

honkar-jdk · 2025-01-10T19:06:41Z

ICC_Profile.setData(..) does validation of the specified tag contents and throws an exception if it is not valid. But if the tag represents the header, at least some of the validation is lazy, occurring only when the data is used, leading to unexpected exceptions at a later time. The check should be done up-front when the data is set, as in other cases.

verifyHeader(byte[] data)is called when header data is being updated and the following fields are validated according to the ICC Spec Document. [1] Pg#19.

Profile/Device class
Color Space
Rendering Intent
PCS
Header Size check (ICC Header Size = 128 bytes)

These validation checks are added to ICC_Profile.getInstance(..) & ICC_Profile.setData(..) methods.

Reference: [1] https://www.color.org/specification/ICC.1-2022-05.pdf

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Warning

⚠️ Patch contains a binary file (test/jdk/java/awt/color/ICC_Profile/ValidateICCHeaderData/invalidSRGB.icc)

Issue

JDK-8347377: Add validation checks for ICC_Profile header fields (Bug - P3)

Reviewers

Phil Race (@prrace - Reviewer)
Jayathirth D V (@jayathirthrao - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23044/head:pull/23044
$ git checkout pull/23044

Update a local copy of the PR:
$ git checkout pull/23044
$ git pull https://git.openjdk.org/jdk.git pull/23044/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23044

View PR using the GUI difftool:
$ git pr show -t 23044

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23044.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-01-10T19:07:59Z

👋 Welcome back honkar! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-01-10T19:08:31Z

@honkar-jdk This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8347377: Add validation checks for ICC_Profile header fields

Reviewed-by: prr, jdv

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 210 new commits pushed to the master branch:

d9d2e19: 8348365: Bad format string in CLDRDisplayNamesTest
59e7509: 8348301: Remove unused Reference.waitForReferenceProcessing break-ins in tests
605b53e: 8348299: Update List/ItemEventTest/ItemEventTest.java
cba0f78: 8348387: Add fixpath if needed for user-supplied tools
44e5cca: 8348391: Keep case if possible for TOPDIR
7460a0a: 8348392: Make claims "other matches are possible" even when that is not true
5cc690d: 8347994: Add additional diagnostics to macOS failure handler to assist with diagnosing MCast test failures
c00557f: 8345049: Remove the jmx.tabular.data.hash.map compatibility property
8b46db0: 8345045: Remove the jmx.remote.x.buffer.size JMX notification property
119899b: 8345048: Remove the jmx.extend.open.types compatibility property
... and 200 more: https://git.openjdk.org/jdk/compare/931914af76932c9b91fc9affd55d24b2562c72d2...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-01-10T19:09:22Z

@honkar-jdk The following label will be automatically applied to this pull request:

client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-01-10T19:12:55Z

Webrevs

src/java.desktop/share/classes/java/awt/color/ICC_ColorSpace.java

src/java.desktop/share/classes/java/awt/color/ICC_Profile.java

mrserb · 2025-01-13T19:10:00Z

src/java.desktop/share/classes/java/awt/color/ICC_Profile.java

@@ -790,6 +791,12 @@ public static ICC_Profile getInstance(byte[] data) {
        } catch (CMMException c) {
            throw new IllegalArgumentException("Invalid ICC Profile Data");
        }
+
+        if (p != null) {


If it possible to get null here we should thrown an exception, but I think we thrown that exception already in the native.

it is probably better to add this validation into ProfileDataVerifier.verify(data), and check it even before .getModule().loadProfile(data)

This requires making the new method verifyHeader()public so that it can be used in ProfileDataVerifier.verify(data) as follows.

byte[] theHeader = new byte[HEADER_SIZE]; System.arraycopy(data,0, theHeader, 0, HEADER_SIZE); ICC_Profile.verifyHeader(theHeader);

or it can be added before .getModule().loadProfile(data) within ICC_Profile.getInstance() and this keeps verifyHeader() private.

public static ICC_Profile getInstance(byte[] data) { ProfileDataVerifier.verify(data); Profile p; try { byte[] theHeader = new byte[HEADER_SIZE]; System.arraycopy(data, 0, theHeader, 0, HEADER_SIZE); verifyHeader(theHeader); p = CMSManager.getModule().loadProfile(data); } catch (CMMException c) { throw new IllegalArgumentException("Invalid ICC Profile Data"); }

@prrace Your suggestion on whether to have verifyHeader() as private or public method? If we decide to make it public then a CSR is required.

@mrserb I have changed it so that verifyHeader is called before CMSManager.getModule().loadProfile(data);
Validation checks cannot be moved to ProfileDataVerifier without making verifyHeader() public.

NOTE: ProfileDataVerifier.verify() verifies entire profile data and not just the header and at this point we have not created a profile yet.

mrserb · 2025-01-13T19:13:16Z

src/java.desktop/share/classes/java/awt/color/ICC_Profile.java

+                    icSaturation, icAbsoluteColorimetric -> {
+                return true;
+            }
+            default -> throw new IllegalArgumentException("Unknown Rendering Intent");


how it is handled by the lcms library? don't we need to ignore unknown intents(and other parameters) and lets lcms decide what to do?

@mrserb
Non-header data are updated using cooked approach (and validated by LCMS) whereas header data are updated using raw LCMS APIs hence require additional validation before setData() is called (On native side it is handled here: setTagDataNative() in LCMS.c).

Without the fix, if invalid rendering intent, PCS, ColorSpace or Device class is updated using setData() it does not throw IAE.

don't we need to ignore unknown intents(and other parameters) and lets lcms decide what to do?

Yes, LCMS ignores invalid header data and updates the profile, this can cause exceptions later on for instance when the modified profile is used to create BufferedImage which can be prevented by adding checks in setData() and restricting updates to only allowed values as specified in ICC Spec Doc.

how it is handled by the lcms library? don't we need to ignore unknown intents(and other parameters) and lets lcms decide what to do?

The ICC spec. defines only these 4 intents, so I don't see a problem here.

@mrserb Non-header data are updated using cooked approach (and validated by LCMS) whereas header data are updated using raw LCMS APIs hence require additional validation before setData() is called (On native side it is handled here: setTagDataNative() in LCMS.c).

Then probably we can use approach similar to 8282577: f66070b and try to rely on lcms for validation.

Did the upstream provide any feedback/comments about this validation?

BTW

Typically, the user or application will set the rendering intent dynamically at runtime or embedding time. Therefore, this flag may not have any meaning until the profile is used in some context, e.g. in a DeviceLink or an embedded source profile."

Why this usecase should not be covered by the java? As of the current version of patch it will not be possible to load the profile and then set the intent, right?

I don't think this is right. It's not a javase spec, and there may be a reason why the most common library for icc profiles accepts thad data. We shouldn't be more strict than that, it will limit java applications compared to the alternatives.

The path that you mentioned in LCMSTransformfor RenderingIntent involves creating a new color space transform using 2 or more profiles and does not involve creating or updating a profile.
Moreover when a random value is added for rendering intent in LCMSTransform, LCMS throws CMMException. If LCMS validates the Rendering Intent while creating a new color space transform then wouldn't it be better to validate it while creating/updating a ICC_Profile?

And as for the different values specified in ICC_Spec vs LCMS API doc, @prrace confirmed that jdk is required to follow ICC_Spec. In other words the color mgnt engine can have different implementations (LCMS or KCMS) but we need to follow the ICC Specification.

Did the upstream provide any feedback/comments about this validation?

Discussed with @prrace, in this case it was decided that contacting upstream is not necessary since we are choosing to do validations in any case (whether or not LCMS validates).

@mrserb

there may be a reason why the most common library for icc profiles accepts thad data. We shouldn't be more strict than that, it will limit java applications compared to the alternatives.

I see your point but color management engines may have different implementations (LCMS or KCMS) and ICC Specification is a standard.

I see your point but color management engines may have different implementations (LCMS or KCMS) and ICC Specification is a standard.

I am pretty sure the lcms/kcms follow the standard, because of the next statement in the "ICC.1-2022-05.pdf"

Any colour management system, application, utility or device driver that claims conformance with this ICC specification shall have the ability to read the profiles as they are defined in this ICC specification. Any profile-generating software and/or hardware that claims conformance with this ICC specification shall have the ability to create profiles as they are defined in this ICC specification.

Since you already mentioned "non-ICC intent", please specify the part of the specification in ICC.1-2022-05 where their use is prohibited.

test/jdk/java/awt/color/ICC_Profile/ValidateICCHeaderData.java

src/java.desktop/share/classes/java/awt/color/ICC_ColorSpace.java

honkar-jdk · 2025-01-18T00:06:08Z

src/java.desktop/share/classes/java/awt/color/ICC_Profile.java

@@ -786,10 +787,15 @@ public static ICC_Profile getInstance(byte[] data) {
        ProfileDataVerifier.verify(data);
        Profile p;
        try {
+            byte[] theHeader = new byte[HEADER_SIZE];
+            System.arraycopy(data, 0, theHeader, 0, HEADER_SIZE);


We won't run into ArrayIndexOutOfBoundsException here since the incoming data array size is already being verified in ProfileDataVerifier.verify(data).

honkar-jdk

@mrserb Can you please re-review the latest code and comments?

test/jdk/java/awt/color/ICC_Profile/ValidateICCHeaderData/ValidateICCHeaderData.java

honkar-jdk · 2025-01-23T16:41:38Z

@jayathirthrao @aivanov-jdk Can you please have another look and review the updated code when you get a chance?

mrserb · 2025-01-24T01:03:35Z

src/java.desktop/share/classes/java/awt/color/ICC_Profile.java

+        if (data == null || data.length < HEADER_SIZE) {
+            throw new IllegalArgumentException("Invalid header data");
+        }
+        getProfileClass(data);


Can any of these fields have custom values (not covered by java constants inside iccCStoJCS) that can still be used for color transformation?

turbanoff · 2025-01-25T12:38:53Z

src/java.desktop/share/classes/java/awt/color/ICC_Profile.java

+         * shall be set to zero. Thus, we are ignoring two most significant
+         * bytes here. Please refer ICC Spec Document for more details.
+         */
+        int renderingIntent = ((header[index+2] & 0xff) <<  8) |


Suggested change

int renderingIntent = ((header[index+2] & 0xff) << 8) |

int renderingIntent = ((header[index+2] & 0xff) << 8) |

honkar-jdk added 2 commits January 10, 2025 10:02

validation checks

f17cdd6

test added

3ef30ad

openjdk bot added the rfr Pull request is ready for review label Jan 10, 2025

openjdk bot added the client [email protected] label Jan 10, 2025

line lengths, spacing

550506d

honkar-jdk commented Jan 10, 2025

View reviewed changes

src/java.desktop/share/classes/java/awt/color/ICC_ColorSpace.java Show resolved Hide resolved

indentation

d3a4b8b

mrserb reviewed Jan 13, 2025

View reviewed changes

IAE exception msg updated

ffacf6f

mrserb reviewed Jan 14, 2025

View reviewed changes

test/jdk/java/awt/color/ICC_Profile/ValidateICCHeaderData.java Outdated Show resolved Hide resolved

honkar-jdk commented Jan 14, 2025

View reviewed changes

src/java.desktop/share/classes/java/awt/color/ICC_ColorSpace.java Show resolved Hide resolved

honkar-jdk added 2 commits January 15, 2025 15:35

added getInstance() test cases

8243687

moved verifyHeader()

04880f6

honkar-jdk commented Jan 18, 2025

View reviewed changes

honkar-jdk added 4 commits January 21, 2025 11:35

test case added for deserialization

a5201b5

typo

b31900b

updated test summary, jtreg tag

78500c2

Runtime Exception added

5aa82b2

prrace reviewed Jan 21, 2025

View reviewed changes

test/jdk/java/awt/color/ICC_Profile/ValidateICCHeaderData/ValidateICCHeaderData.java Outdated Show resolved Hide resolved

prrace approved these changes Jan 22, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 22, 2025

jayathirthrao approved these changes Jan 23, 2025

View reviewed changes

mrserb reviewed Jan 24, 2025

View reviewed changes

turbanoff reviewed Jan 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDK-8347377 : Add validation checks for ICC_Profile header fields #23044

JDK-8347377 : Add validation checks for ICC_Profile header fields #23044

honkar-jdk commented Jan 10, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 10, 2025

openjdk bot commented Jan 10, 2025 •

edited

Loading

openjdk bot commented Jan 10, 2025

mlbridge bot commented Jan 10, 2025 •

edited

Loading

mrserb Jan 13, 2025

mrserb Jan 13, 2025

honkar-jdk Jan 13, 2025 •

edited

Loading

honkar-jdk Jan 18, 2025

mrserb Jan 13, 2025

honkar-jdk Jan 13, 2025 •

edited

Loading

honkar-jdk Jan 13, 2025 •

edited

Loading

prrace Jan 13, 2025 •

edited

Loading

mrserb Jan 13, 2025

mrserb Jan 24, 2025 •

edited

Loading

mrserb Jan 24, 2025

honkar-jdk Jan 24, 2025 •

edited

Loading

honkar-jdk Jan 24, 2025 •

edited

Loading

mrserb Jan 25, 2025

honkar-jdk Jan 18, 2025

honkar-jdk left a comment

honkar-jdk commented Jan 23, 2025

mrserb Jan 24, 2025

turbanoff Jan 25, 2025

	int renderingIntent = ((header[index+2] & 0xff) << 8) \|
	int renderingIntent = ((header[index+2] & 0xff) << 8) \|

JDK-8347377 : Add validation checks for ICC_Profile header fields #23044

Are you sure you want to change the base?

JDK-8347377 : Add validation checks for ICC_Profile header fields #23044

Conversation

honkar-jdk commented Jan 10, 2025 • edited by openjdk bot Loading

Progress

Warning

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 10, 2025

openjdk bot commented Jan 10, 2025 • edited Loading

openjdk bot commented Jan 10, 2025

mlbridge bot commented Jan 10, 2025 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honkar-jdk Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honkar-jdk Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

honkar-jdk Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

prrace Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrserb Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honkar-jdk Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

honkar-jdk Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honkar-jdk left a comment

Choose a reason for hiding this comment

honkar-jdk commented Jan 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honkar-jdk commented Jan 10, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jan 10, 2025 •

edited

Loading

mlbridge bot commented Jan 10, 2025 •

edited

Loading

honkar-jdk Jan 13, 2025 •

edited

Loading

honkar-jdk Jan 13, 2025 •

edited

Loading

honkar-jdk Jan 13, 2025 •

edited

Loading

prrace Jan 13, 2025 •

edited

Loading

mrserb Jan 24, 2025 •

edited

Loading

honkar-jdk Jan 24, 2025 •

edited

Loading

honkar-jdk Jan 24, 2025 •

edited

Loading