OpenJDK › OpenJDK Core Libraries

RFR: JDK-8262199: TOCTOU in jli args.c

Classic

List

Threaded

20 messages Options

Matthias Baesken

RFR: JDK-8262199: TOCTOU in jli args.c

Sonar reports a finding in args.c, where a file check is done .
Stat performs a check on file, and later fopen is called on the file :
https://sonarcloud.io/project/issues?id=shipilev_jdk&languages=c&open=AXck8CL0BBG2CXpcnhtM&resolved=false&types=VULNERABILITY

The coding could be slightly rewritten so that the potential TOCTOU is removed (however I do not think that it is such a big issue).

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

Commit messages:
- JDK-8262199

Changes: https://git.openjdk.java.net/jdk/pull/2692/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2692&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8262199
Stats: 32 lines in 1 file changed: 12 ins; 16 del; 4 mod
Patch: https://git.openjdk.java.net/jdk/pull/2692.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2692/head:pull/2692

PR: https://git.openjdk.java.net/jdk/pull/2692

Christoph Langer

Re: RFR: JDK-8262199: TOCTOU in jli args.c

On Tue, 23 Feb 2021 13:58:03 GMT, Matthias Baesken <[hidden email]> wrote:

> Sonar reports a finding in args.c, where a file check is done .
> Stat performs a check on file, and later fopen is called on the file :
> https://sonarcloud.io/project/issues?id=shipilev_jdk&languages=c&open=AXck8CL0BBG2CXpcnhtM&resolved=false&types=VULNERABILITY
>
> The coding could be slightly rewritten so that the potential TOCTOU is removed (however I do not think that it is such a big issue).

This looks good in general. Do you know whether there's a jtreg test that stresses arg files?

src/java.base/share/native/libjli/args.c line 361:

> 359: if (fptr != NULL) fclose(fptr);
> 360: exit(1);
> 361: }

Can you insert a blank line here?

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

Changes requested by clanger (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2692

Matthias Baesken

Re: RFR: JDK-8262199: TOCTOU in jli args.c

On Tue, 23 Feb 2021 14:05:15 GMT, Christoph Langer <[hidden email]> wrote:

> This looks good in general. Do you know whether there's a jtreg test that stresses arg files?

There are tests dealing with args files at test/jdk/tools/launcher/ , e.g. there is test/jdk/tools/launcher/ArgsFileTest.java .

Best regards, Matthias

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

PR: https://git.openjdk.java.net/jdk/pull/2692

Matthias Baesken

Re: RFR: JDK-8262199: TOCTOU in jli args.c [v2]

In reply to this post by Matthias Baesken

> Sonar reports a finding in args.c, where a file check is done .
> Stat performs a check on file, and later fopen is called on the file :
> https://sonarcloud.io/project/issues?id=shipilev_jdk&languages=c&open=AXck8CL0BBG2CXpcnhtM&resolved=false&types=VULNERABILITY
>
> The coding could be slightly rewritten so that the potential TOCTOU is removed (however I do not think that it is such a big issue).

Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision:

Small changes

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

Changes:
- all: https://git.openjdk.java.net/jdk/pull/2692/files
- new: https://git.openjdk.java.net/jdk/pull/2692/files/78467273..8b106a14

Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2692&range=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2692&range=00-01

Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/2692.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2692/head:pull/2692

PR: https://git.openjdk.java.net/jdk/pull/2692

Christoph Langer

Re: RFR: JDK-8262199: TOCTOU in jli args.c [v2]

In reply to this post by Matthias Baesken

On Tue, 23 Feb 2021 14:23:59 GMT, Matthias Baesken <[hidden email]> wrote:

> > This looks good in general. Do you know whether there's a jtreg test that stresses arg files?
>
> There are tests dealing with args files at test/jdk/tools/launcher/ , e.g. there is test/jdk/tools/launcher/ArgsFileTest.java .
>
> Best regards, Matthias

OK, I added label noreg-sqe to the bug then.

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

PR: https://git.openjdk.java.net/jdk/pull/2692

Christoph Langer

Re: RFR: JDK-8262199: TOCTOU in jli args.c [v2]

In reply to this post by Matthias Baesken

On Tue, 23 Feb 2021 14:30:17 GMT, Matthias Baesken <[hidden email]> wrote:

>> Sonar reports a finding in args.c, where a file check is done .
>> Stat performs a check on file, and later fopen is called on the file :
>> https://sonarcloud.io/project/issues?id=shipilev_jdk&languages=c&open=AXck8CL0BBG2CXpcnhtM&resolved=false&types=VULNERABILITY
>>
>> The coding could be slightly rewritten so that the potential TOCTOU is removed (however I do not think that it is such a big issue).
>
> Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision:
>
> Small changes

Marked as reviewed by clanger (Reviewer).

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

PR: https://git.openjdk.java.net/jdk/pull/2692

Alan Bateman-2

Re: RFR: JDK-8262199: issue in jli args.c [v2]

In reply to this post by Christoph Langer

On Tue, 23 Feb 2021 14:04:29 GMT, Christoph Langer <[hidden email]> wrote:

>> Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision:
>>
>> Small changes
>
> src/java.base/share/native/libjli/args.c line 361:
>
>> 359: if (fptr != NULL) fclose(fptr);
>> 360: exit(1);
>> 361: }
>
> Can you insert a blank line here?

This function is "optionally report, optionally fclose, and then exit". Have you tried reducing it to reportAndExit and fclose inline in expandArgFile to avoid it doing 3 things?

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

PR: https://git.openjdk.java.net/jdk/pull/2692

Matthias Baesken

Re: RFR: JDK-8262199: issue in jli args.c [v2]

On Tue, 23 Feb 2021 19:24:31 GMT, Alan Bateman <[hidden email]> wrote:

> This function is "optionally report, optionally fclose, and then exit". Have you tried reducing it to reportAndExit and fclose inline in expandArgFile to avoid it doing 3 things?

hi Alan , thanks for your remark , I did not do that because I would need to copy both lines (fclose + exit) to all callers.
On the other hand I wonder - wouldn't it be possible to avoid the fclose before the exit(1) ?
I assume the closing is done on exit anyway ?

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

PR: https://git.openjdk.java.net/jdk/pull/2692

Alan Bateman-2

Re: RFR: JDK-8262199: issue in jli args.c [v2]

On Thu, 25 Feb 2021 08:54:48 GMT, Matthias Baesken <[hidden email]> wrote:

>> This function is "optionally report, optionally fclose, and then exit". Have you tried reducing it to reportAndExit and fclose inline in expandArgFile to avoid it doing 3 things?
>
>> This function is "optionally report, optionally fclose, and then exit". Have you tried reducing it to reportAndExit and fclose inline in expandArgFile to avoid it doing 3 things?
>
> hi Alan , thanks for your remark , I did not do that because I would need to copy both lines (fclose + exit) to all callers.
> On the other hand I wonder - wouldn't it be possible to avoid the fclose before the exit(1) ?
> I assume the closing is done on exit anyway ?

That sounds okay to me, the process is exiting, as you say.

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

PR: https://git.openjdk.java.net/jdk/pull/2692

Matthias Baesken

Re: RFR: JDK-8262199: issue in jli args.c [v3]

In reply to this post by Matthias Baesken

> Sonar reports a finding in args.c, where a file check is done .
> Stat performs a check on file, and later fopen is called on the file .
>
> The coding could be slightly rewritten so that the potential issue is removed (however I do not think that it is such a big issue).

Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision:

Remove fclose before exit

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

Changes:
- all: https://git.openjdk.java.net/jdk/pull/2692/files
- new: https://git.openjdk.java.net/jdk/pull/2692/files/8b106a14..b5eeca4f

Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2692&range=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2692&range=01-02

Stats: 5 lines in 1 file changed: 0 ins; 1 del; 4 mod
Patch: https://git.openjdk.java.net/jdk/pull/2692.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2692/head:pull/2692

PR: https://git.openjdk.java.net/jdk/pull/2692

Christoph Langer

Re: RFR: JDK-8262199: issue in jli args.c [v3]

On Thu, 25 Feb 2021 15:40:00 GMT, Matthias Baesken <[hidden email]> wrote:

>> Sonar reports a finding in args.c, where a file check is done .
>> Stat performs a check on file, and later fopen is called on the file .
>>
>> The coding could be slightly rewritten so that the potential issue is removed (however I do not think that it is such a big issue).
>
> Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision:
>
> Remove fclose before exit

Changes requested by clanger (Reviewer).

src/java.base/share/native/libjli/args.c line 378:

> 376: if (st.st_size > MAX_ARGF_SIZE) {
> 377: JLI_ReportMessage(CFG_ERROR10, MAX_ARGF_SIZE);
> 378: reportAndExit(NULL, NULL);

This should be just one statement,
reportAndExit(CFG_ERROR10, MAX_ARGF_SIZE);
or?

src/java.base/share/native/libjli/args.c line 358:

> 356:
> 357: static void reportAndExit(const char* fmt, const char* arg) {
> 358: if (fmt != NULL) JLI_ReportMessage(fmt, arg);

the if (fmt != NULL) check wouldn't be necessary here if you fix the other location with reportAndExit(NULL, NULL), I think

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

PR: https://git.openjdk.java.net/jdk/pull/2692

Matthias Baesken

Re: RFR: JDK-8262199: issue in jli args.c [v4]

In reply to this post by Matthias Baesken

> Sonar reports a finding in args.c, where a file check is done .
> Stat performs a check on file, and later fopen is called on the file .
>
> The coding could be slightly rewritten so that the potential issue is removed (however I do not think that it is such a big issue).

Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision:

Simplify coding

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

Changes:
- all: https://git.openjdk.java.net/jdk/pull/2692/files
- new: https://git.openjdk.java.net/jdk/pull/2692/files/b5eeca4f..39faea43

Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2692&range=03
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2692&range=02-03

Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
Patch: https://git.openjdk.java.net/jdk/pull/2692.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2692/head:pull/2692

PR: https://git.openjdk.java.net/jdk/pull/2692

Matthias Baesken

Re: RFR: JDK-8262199: issue in jli args.c [v3]

In reply to this post by Christoph Langer

On Thu, 25 Feb 2021 16:07:36 GMT, Christoph Langer <[hidden email]> wrote:

>> Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision:
>>
>> Remove fclose before exit
>
> src/java.base/share/native/libjli/args.c line 378:
>
>> 376: if (st.st_size > MAX_ARGF_SIZE) {
>> 377: JLI_ReportMessage(CFG_ERROR10, MAX_ARGF_SIZE);
>> 378: reportAndExit(NULL, NULL);
>
> This should be just one statement,
> reportAndExit(CFG_ERROR10, MAX_ARGF_SIZE);
> or?

I think that did not work because it does not fit the param types of reportAndExit but I can simplify it otherwise.

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

PR: https://git.openjdk.java.net/jdk/pull/2692

Alan Bateman-2

Re: RFR: JDK-8262199: issue in jli args.c [v4]

In reply to this post by Matthias Baesken

On Thu, 25 Feb 2021 16:44:00 GMT, Matthias Baesken <[hidden email]> wrote:

>> Sonar reports a finding in args.c, where a file check is done .
>> Stat performs a check on file, and later fopen is called on the file .
>>
>> The coding could be slightly rewritten so that the potential issue is removed (however I do not think that it is such a big issue).
>
> Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision:
>
> Simplify coding

Thanks for the updates, the latest looks clean to me.

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

Marked as reviewed by alanb (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2692

Christoph Langer

Re: RFR: JDK-8262199: issue in jli args.c [v3]

In reply to this post by Matthias Baesken

On Thu, 25 Feb 2021 16:39:25 GMT, Matthias Baesken <[hidden email]> wrote:

>> src/java.base/share/native/libjli/args.c line 378:
>>
>>> 376: if (st.st_size > MAX_ARGF_SIZE) {
>>> 377: JLI_ReportMessage(CFG_ERROR10, MAX_ARGF_SIZE);
>>> 378: reportAndExit(NULL, NULL);
>>
>> This should be just one statement,
>> reportAndExit(CFG_ERROR10, MAX_ARGF_SIZE);
>> or?
>
> I think that did not work because it does not fit the param types of reportAndExit but I can simplify it otherwise.

Ah, I see.
Maybe it's a bit bike-sheddy but as you're using reportAndExit only twice now, wouldn't it be less convoluted if you use
JLI_ReportMessage(...);
exit(1);
inline in both places?

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

PR: https://git.openjdk.java.net/jdk/pull/2692

Matthias Baesken

Re: RFR: JDK-8262199: issue in jli args.c [v5]

In reply to this post by Matthias Baesken

> Sonar reports a finding in args.c, where a file check is done .
> Stat performs a check on file, and later fopen is called on the file .
>
> The coding could be slightly rewritten so that the potential issue is removed (however I do not think that it is such a big issue).

Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision:

Replace reportAndExit

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

Changes:
- all: https://git.openjdk.java.net/jdk/pull/2692/files
- new: https://git.openjdk.java.net/jdk/pull/2692/files/39faea43..2da97d61

Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2692&range=04
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2692&range=03-04

Stats: 9 lines in 1 file changed: 2 ins; 5 del; 2 mod
Patch: https://git.openjdk.java.net/jdk/pull/2692.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2692/head:pull/2692

PR: https://git.openjdk.java.net/jdk/pull/2692

Matthias Baesken

Re: RFR: JDK-8262199: issue in jli args.c [v3]

In reply to this post by Christoph Langer

On Thu, 25 Feb 2021 17:46:42 GMT, Christoph Langer <[hidden email]> wrote:

>> I think that did not work because it does not fit the param types of reportAndExit but I can simplify it otherwise.
>
> Ah, I see.
> Maybe it's a bit bike-sheddy but as you're using reportAndExit only twice now, wouldn't it be less convoluted if you use
> JLI_ReportMessage(...);
> exit(1);
> inline in both places?

Yes I agree, see my latest change.

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

PR: https://git.openjdk.java.net/jdk/pull/2692

Christoph Langer

Re: RFR: JDK-8262199: issue in jli args.c [v5]

In reply to this post by Matthias Baesken

On Fri, 26 Feb 2021 08:31:00 GMT, Matthias Baesken <[hidden email]> wrote:

>> Sonar reports a finding in args.c, where a file check is done .
>> Stat performs a check on file, and later fopen is called on the file .
>>
>> The coding could be slightly rewritten so that the potential issue is removed (however I do not think that it is such a big issue).
>
> Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision:
>
> Replace reportAndExit

Looks good now :)

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

Marked as reviewed by clanger (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2692

Alan Bateman-2

Re: RFR: JDK-8262199: issue in jli args.c [v5]

In reply to this post by Matthias Baesken

On Fri, 26 Feb 2021 08:31:00 GMT, Matthias Baesken <[hidden email]> wrote:

>> Sonar reports a finding in args.c, where a file check is done .
>> Stat performs a check on file, and later fopen is called on the file .
>>
>> The coding could be slightly rewritten so that the potential issue is removed (however I do not think that it is such a big issue).
>
> Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision:
>
> Replace reportAndExit

This version looks good too.

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

Marked as reviewed by alanb (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2692

Matthias Baesken

Integrated: JDK-8262199: issue in jli args.c

In reply to this post by Matthias Baesken

On Tue, 23 Feb 2021 13:58:03 GMT, Matthias Baesken <[hidden email]> wrote:

> Sonar reports a finding in args.c, where a file check is done .
> Stat performs a check on file, and later fopen is called on the file .
>
> The coding could be slightly rewritten so that the potential issue is removed (however I do not think that it is such a big issue).

This pull request has now been integrated.

Changeset: d7efb4cc
Author: Matthias Baesken <[hidden email]>
URL: https://git.openjdk.java.net/jdk/commit/d7efb4cc
Stats: 25 lines in 1 file changed: 8 ins; 15 del; 2 mod

8262199: issue in jli args.c

Reviewed-by: clanger, alanb

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

PR: https://git.openjdk.java.net/jdk/pull/2692