disallow absolute file path in oras push and oras attach by default

[qweeah]

What is the version of your ORAS CLI

oras 1.0.0

What would you like to be added?

By default, disable pushing files with absolute file path in oras push and oras attach, add a new flag, like --allow-absolute-path to support glass-breaking scenarios.

The artifact packed via oras attach/push --allow-absolute-path can only be pulled via oras pull --T/allow-path-traversal

Why is this needed for ORAS?

Absolutely pathed layers are dangerous, since

1. File might be written into any folder outside of the working directory.
2. The allowed characters in files names are different between Linux and Windows. Artifacts packed in one platform may not be pulled in another platform.

Are you willing to submit PRs to contribute to this feature?

• Yes, I am willing to implement it.

[qweeah]

@suganyas I am not sure packaging an absolutely pathed artifact is intended in your case. If it's not, maybe oras should guide our users not to produce those artifacts in 2.0?

[suganyas]

Hi @qweeah thanks for looking into it . I just tried to push an artifact file from linux environment from a absolute path or from different directory. The path is implicitly taken by the oras cli or oras sdk when I pushed it. Like the file was in a directory /home/vts/1/a.exe. and I pushed from /home/test. I am ok if the push fails stating me that the file is not in the current directory and for security reasons you have to be in same working directory. But the push passes and pull fails . So disallow is a much better option. I am sure when I used ORAS python SDK it did fail with error stating as here https://github.com/oras-project/oras-py/blob/209c9b98043a00d1b04789cc2967ca7021dc5b2e/oras/provider.py#L651 . The CLI should have same behaviour as SDK. Then push and pull are coherent. It can be a bad experience when push is ok and pull fails and when different people do it can be hard for people to understand why it fails

[suganyas]

@qweeah I am happy to contribute or make changes according to our decision

[qweeah]

@suganyas My idea is that it's better to prevent user from using absolute path. @FeynmanZhou @shizhMSFT @TerryHowe what do you think?

P.S. This issue introduces breaking change and need to be enabled for ORAS 2.0 release.

[FeynmanZhou]

This will change the default behavior of oras push and oras attach so I agree that it should be planned in ORAS v2.0.0.

My idea is that it's better to prevent user from using absolute path.

I think our intention is to recommend users use a relative path by default but provide an option for users who stick to using an absolute path across different targets. We need to clearly define the error messages when users use the absolute path when pushing/attaching without specifying the --allow-absolute-path or pulling from an absolute path without --T/allow-path-traversal.

[suganyas]

I have a question @FeynmanZhou @qweeah on disallow abs path by default. This means push will be successful for an abs path only when --allow-absolute-path path is enabled during oras push?? It will be a new flag added? I don't see it now in oras and when pull fails with path traversal error tell them to use allow-path-traversal. Am I understanding it right?

[shizhMSFT]

@suganyas The discussion by @FeynmanZhou was for oras v2.0.0. For the current version, there is no restriction on pushing files via an absolute path.

Here are more insights: Using absolute paths with -T is generally insecure and not cross platform friendly.

[suganyas]

@shizhMSFT thanks for the clarification. So these are the changes we need to do in version v2.0.0 (prevent default push from absolute path and proper error message to help to identify the issue because pull and push can be done in different systems and persons)

• Introduce --allow-absolute-path flag for push
• By default (without the --allow-absolute-path) oras push shud fail if the file is not in current working directory. It means with --allow-absolute-path user intentional pushed it from abs path. We will also add a note after pushing use --allow-path-traversal flag to pull and also might not work in cross platforms
• When a pull fails with path traversal error print use -T/--allow-path-traversal flag to pull fine.
  As I am keen to work on this change should I just fork and start working on it? or can you guide me what is the process to get this into v2.0.0. @qweeah @FeynmanZhou @shizhMSFT

[shizhMSFT]

@qweeah @suganyas After digging into the code, I noticed that we just found a bug.

There is already a flag named --disable-path-validation for oras push. Thus, we don't need to introduce a new flag named `--allow-absolute-path``. /cc @FeynmanZhou

      --disable-path-validation                    skip path validation

However, this flag is incorrectly implemented. By looking at the current implementation:

oras/cmd/oras/root/push.go

Line 142 in 365ad23

store.AllowPathTraversalOnWrite = opts.PathValidationDisabled

At the first glance, it seems correct. However, it is wrong since the file store is the source for reading and thus AllowPathTraversalOnWrite takes no effect.

We need to fix it by properly implementing the flag --disable-path-validation for oras push. Since it is a bug fix, we can fix it in v1.

[shizhMSFT]

@qweeah oras attach has the same bug.

[qweeah]

@shizhMSFT Thanks for pointing out. I forgot we already have this flag. Created an issue #983

The question is: If we fixed this bug, what default value should we give it? Since --disable-path-validation is set to false by default, which will break user scenarios that requires pushing artifacts with absolute path. @FeynmanZhou

[shizhMSFT]

@qweeah It is a bug not a feature. The interface and default value are by design but the behaviour of the implementation mismatches the design. Therefore, fixing this bug is not considered as a breaking change.

[qweeah]

@qweeah It is a bug not a feature. The interface and default value are by design but the behaviour of the implementation mismatches the design. Therefore, fixing this bug is not considered as a breaking change.

Path validation flag requires checking on whether the input file paths are in the sub-folder of current working directory. What we want to achieve is disable pushing artifacts containing absolute path by default. There is a subtle difference between those two, like: What if the file path is in current working directory but is presented as absolute format? Should we allow pushing of it

I think for the scope of #983, we can fix the path validation and allow pushing such files. But in 2.0, we should disable that. @shizhMSFT

[shizhMSFT]

What if the file path is in current working directory but is presented as absolute format? Should we allow pushing of it

No, we should not allowing pushing it by default as it is not pullable by default.

[shizhMSFT]

Marking this issue as a duplicate of #983