Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove domain restrictions on URL protocol imports #5347

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

isycat
Copy link

@isycat isycat commented Nov 20, 2024

When importing a file via bambustudio://open/?file=bla, the URL is now allowed even if it does not match makerworld, amazonaws.com or aliyuncs.com

When importing a file via bambustudio://open/?file=bla, the URL is now allowed even if it does not match makerworld, amazonaws.com or aliyuncs.com
@lanewei120
Copy link
Collaborator

@isycat thanks for the patch

I am afraid we can not merge it for the security reason

Without restricting the source of the URL, anyone could use the URL to pass in something like a Trojan or other types of malware.

@NanashiTheNameless
Copy link

@isycat thanks for the patch

I am afraid we can not merge it for the security reason

Without restricting the source of the URL, anyone could use the URL to pass in something like a Trojan or other types of malware.

IMO this seems kinda non-issue, maybe give a toggle for the restrictions and say something like "Warning, when this is disabled links from malicious sites may be loaded, proceed at your own risk" when the user goes to disable them?

@lanewei120
Copy link
Collaborator

Not everyone will carefully read these warning messages,
and even if they do, if they end up clicking "Import" and getting infected by a virus, Bambustudio would still be held responsible.

In fact, what we initially wanted was the way you’ve modified it in this patch
image

@isycat
Copy link
Author

isycat commented Nov 28, 2024

Currently, you can just host the malware on a - random s3 bucket or literally any domain or sub domain starting with "makerworld". Examples:
evilbucket.s3-website-us-east-1.amazonaws.com/malware.stl
makerworldevil.com/malware.stl
makerworld.evil.example.com/malware.stl

If this really is intended to help the user it should be togglable, as suggested above, and check for whitelisted certificates instead of a partial URL check.

Anything short of that is effectively just annoying gatekeeping. Let's not pretend it's a security feature.

@lanewei120
Copy link
Collaborator

yes, you are right, above urls also have risks:)
so I think we should add more checks for the urls to improve the security

In fact, this security risk is already reported by our customer before on the the previous version using "http://" and "https://"

@NanashiTheNameless
Copy link

NanashiTheNameless commented Nov 28, 2024

Currently, you can just host the malware on a - random s3 bucket or literally any domain or sub domain starting with "makerworld". Examples: evilbucket.s3-website-us-east-1.amazonaws.com/malware.stl makerworldevil.com/malware.stl makerworld.evil.example.com/malware.stl

If this really is intended to help the user it should be togglable, as suggested above, and check for whitelisted certificates instead of a partial URL check.

Anything short of that is effectively just annoying gatekeeping. Let's not pretend it's a security feature.

This right here,
lets be honest, this is almost certainly just gatekeeping.
If it was for security it should just be

if (boost::starts_with(input_str, "https://makerworld.com/") {
  download_url = input_str;
}

This is almost certainly a (Misguided) attempt at vendor lock-in.
It should default to above and have a toggle to allow all sources as this commit has it set.

You can show a warning that says "Warning: When this is disabled links from potentially malicious sites are able to be loaded, proceed at your own risk, BambuLab and its affiliates are not responsible for any risk assumed by disabling this feature!"
That IS in fact legally binding in most countries.

Edit: For example https://makerworld.MalwareDomain.invalid/ImVeryEvil-Malware.stl currently is allowed
but https://files.printables.com/media/prints/identifier/stls/otheridentifier/example.stl is not allowed

@NanashiTheNameless
Copy link

Another reason the "security" argument falls apart, A bad actor can also just (mis)use a FOSS CORS proxy and have it serve https://files.printables.com/media/prints/identifier/stls/otheridentifier/example.stl as https://makerworld.evilproxy.invalid/?https://files.printables.com/media/prints/identifier/stls/otheridentifier/example.stl and that would effectively open it up to allowing anything to be loaded.

@lanewei120
Copy link
Collaborator

@NanashiTheNameless
thanks for the suggestion

@isycat
we have a fast discuss internally,
and we suggest to do as follows:

keep the original url judge logic currently, let it go on as before(we will refine more these address check later)
and popup a warning dialog when the url doesn't meet the above
and if user clicks ok, bambu studio will go on to load it

could you do the modifications?

@NanashiTheNameless
Copy link

@NanashiTheNameless thanks for the suggestion

@isycat we have a fast discuss internally, and we suggest to do as follows:

keep the original url judge logic currently, let it go on as before(we will refine more these address check later) and popup a warning dialog when the url doesn't meet the above and if user clicks ok, bambu studio will go on to load it

could you do the modifications?

"popup a warning dialog when the url doesn't meet the above and if user clicks ok, bambu studio will go on to load it"

I approve of this idea, best of both worlds!

@isycat
Copy link
Author

isycat commented Nov 30, 2024

This is a perfect idea, thanks!
I am not sure when/if I will find the time to make the change, so if someone else wants to pick it up, please do.

@chromozonechris
Copy link

hello I need a website that gives adware please

@NanashiTheNameless
Copy link

hello I need a website that gives adware please

What do you mean and how is this relevant?

I mean, you can try using
https://www.wicar.org/
https://canyoublockit.com/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants