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

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

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

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).

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

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
Reply | Threaded
Open this post in threaded view
|

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

Christoph Langer
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
Reply | Threaded
Open this post in threaded view
|

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

Matthias Baesken
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
Reply | Threaded
Open this post in threaded view
|

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

Matthias Baesken
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
Reply | Threaded
Open this post in threaded view
|

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

Christoph Langer
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
Reply | Threaded
Open this post in threaded view
|

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

Christoph Langer
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
Reply | Threaded
Open this post in threaded view
|

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

Alan Bateman-2
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
Reply | Threaded
Open this post in threaded view
|

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

Matthias Baesken
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
Reply | Threaded
Open this post in threaded view
|

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

Alan Bateman-2
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
Reply | Threaded
Open this post in threaded view
|

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

Matthias Baesken
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
Reply | Threaded
Open this post in threaded view
|

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

Christoph Langer
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
Reply | Threaded
Open this post in threaded view
|

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

Matthias Baesken
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
Reply | Threaded
Open this post in threaded view
|

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

Matthias Baesken
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
Reply | Threaded
Open this post in threaded view
|

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

Alan Bateman-2
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
Reply | Threaded
Open this post in threaded view
|

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

Christoph Langer
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
Reply | Threaded
Open this post in threaded view
|

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

Matthias Baesken
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
Reply | Threaded
Open this post in threaded view
|

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

Matthias Baesken
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
Reply | Threaded
Open this post in threaded view
|

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

Christoph Langer
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
Reply | Threaded
Open this post in threaded view
|

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

Alan Bateman-2
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
Reply | Threaded
Open this post in threaded view
|

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

Matthias Baesken
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