You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mxnet.apache.org by GitBox <gi...@apache.org> on 2020/02/14 22:06:15 UTC
[GitHub] [incubator-mxnet] ChaiBapchya opened a new pull request #17597:
move stacktrance print to console rather than system.err
ChaiBapchya opened a new pull request #17597: move stacktrance print to console rather than system.err
URL: https://github.com/apache/incubator-mxnet/pull/17597
## Description ##
Right now with #17465 if the build fails, we print the stacktrace using `error.printStackTrace()` function
a. It is an insecure function that needs to be approved by administrators in Jenkins -> Manage Jenkins -> In-process Script approval
Without the approval it gives SandBox error
```
`org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException: Scripts not permitted to use method java.lang.Throwable getStackTrace`
```
b. However, the `error.printStackTrace()` function sends the report to System.err
It doesn't get printed on the console.
To print on console, it has to be passed as a parameter.
Verified it works by adding a failure case in the try (so as to enter the "catch")
```
try {
update_github_commit_status('PENDING', 'Job has been enqueued')
System.out.println(12 / 0)
}
```
It enters catch and then prints the stack trace for divide by zero exception as expected.
Refer :
http://jenkins.mxnet-ci-dev.amazon-ml.com/job/mxnet-validation/job/apache-unix-cpu/job/remove_print_stacktrace/lastBuild/console
## Checklist ##
### Essentials ###
Please feel free to remove inapplicable items for your PR.
- [ ] Changes are complete (i.e. I finished coding on this PR)
- [ ] All changes have test coverage:
- [ ] Code is well-documented:
- [ ] To the best of my knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change
## Comments ##
Read more about Jenkins Script Security : https://plugins.jenkins.io/script-security/
How to handle printStackTrace - https://stackoverflow.com/questions/12095378/difference-between-e-printstacktrace-and-system-out-printlne
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] ChaiBapchya commented on issue #17597: [CI] Move
stacktrace print to console rather than system.err
Posted by GitBox <gi...@apache.org>.
ChaiBapchya commented on issue #17597: [CI] Move stacktrace print to console rather than system.err
URL: https://github.com/apache/incubator-mxnet/pull/17597#issuecomment-587244013
@marcoabreu The security concerns are well-founded.
@access2rohit Do you mind explaining here why it was chosen to add printStackTrace to console despite its security concerns? Is it coz
a. Code for CI as well as MXNet codebase in general is open-source & public
b. Errors revealed in stacktrace don't quite give away much info about the protected system.
Did I miss something?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] marcoabreu commented on issue #17597: [CI] Move
stacktrace print to console rather than system.err
Posted by GitBox <gi...@apache.org>.
marcoabreu commented on issue #17597: [CI] Move stacktrace print to console rather than system.err
URL: https://github.com/apache/incubator-mxnet/pull/17597#issuecomment-586679086
Have you verified the security implications? Usually, these restrictions are in place for a reason - I'd recommend to first check why the developers considered that function security critical before moving forward.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] marcoabreu commented on issue #17597: [CI] Move
stacktrace print to console rather than system.err
Posted by GitBox <gi...@apache.org>.
marcoabreu commented on issue #17597: [CI] Move stacktrace print to console rather than system.err
URL: https://github.com/apache/incubator-mxnet/pull/17597#issuecomment-587781637
Sounds good :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] marcoabreu commented on issue #17597: [CI] Move
stacktrace print to console rather than system.err
Posted by GitBox <gi...@apache.org>.
marcoabreu commented on issue #17597: [CI] Move stacktrace print to console rather than system.err
URL: https://github.com/apache/incubator-mxnet/pull/17597#issuecomment-587320012
While MXNet might be open source, the Jenkins master is still are security relevant system. It contains credentials, has a user facing website and publishes artifacts.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] ChaiBapchya commented on issue #17597: [CI] Move
stacktrace print to console rather than system.err
Posted by GitBox <gi...@apache.org>.
ChaiBapchya commented on issue #17597: [CI] Move stacktrace print to console rather than system.err
URL: https://github.com/apache/incubator-mxnet/pull/17597#issuecomment-587654810
Yes It is a 404 due to someone changing the job config. Removed the link from GitHub anyway to prevent any sec issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] ChaiBapchya commented on issue #17597: [CI] Move
stacktrace print to console rather than system.err
Posted by GitBox <gi...@apache.org>.
ChaiBapchya commented on issue #17597: [CI] Move stacktrace print to console rather than system.err
URL: https://github.com/apache/incubator-mxnet/pull/17597#issuecomment-587125716
Um it's a common practice to get the stack trace of errors and render them on Jenkins. A cursory glance on google search revealed lots of users doing that. I'm not sure if printing stacktrace poses security risk.
Can you be more specific? Unable to understand exactly what's the contention.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] ChaiBapchya commented on issue #17597: move
stacktrance print to console rather than system.err
Posted by GitBox <gi...@apache.org>.
ChaiBapchya commented on issue #17597: move stacktrance print to console rather than system.err
URL: https://github.com/apache/incubator-mxnet/pull/17597#issuecomment-586491889
@mxnet-label-bot add [pr-awaiting-review]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] ChaiBapchya commented on issue #17597: [CI]
Remove print stacktrace (due to security concerns)
Posted by GitBox <gi...@apache.org>.
ChaiBapchya commented on issue #17597: [CI] Remove print stacktrace (due to security concerns)
URL: https://github.com/apache/incubator-mxnet/pull/17597#issuecomment-596161812
@mxnet-label-bot add [pr-awaiting-merge]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] marcoabreu commented on issue #17597: [CI] Move
stacktrace print to console rather than system.err
Posted by GitBox <gi...@apache.org>.
marcoabreu commented on issue #17597: [CI] Move stacktrace print to console rather than system.err
URL: https://github.com/apache/incubator-mxnet/pull/17597#issuecomment-587662236
Mate, it doesn't matter who did what. You've identified a security issue and it's now on someone to fix it.
The alternative is to have the PMC write an email to security@amazon.com and have a sev2 filed. I don't understand why we're still discussing this matter.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] marcoabreu commented on issue #17597: [CI] Move
stacktrace print to console rather than system.err
Posted by GitBox <gi...@apache.org>.
marcoabreu commented on issue #17597: [CI] Move stacktrace print to console rather than system.err
URL: https://github.com/apache/incubator-mxnet/pull/17597#issuecomment-587762528
I understand and thanks for bringing it up. Sorry for being so hard on this, but it's difficult to compromise on security.
Has the whitelist entry been removed?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] aaronmarkham commented on issue #17597: [CI] Move
stacktrace print to console rather than system.err
Posted by GitBox <gi...@apache.org>.
aaronmarkham commented on issue #17597: [CI] Move stacktrace print to console rather than system.err
URL: https://github.com/apache/incubator-mxnet/pull/17597#issuecomment-587595235
@ChaiBapchya This [reference](http://jenkins.mxnet-ci-dev.amazon-ml.com/job/mxnet-validation/job/apache-unix-cpu/job/remove_print_stacktrace/lastBuild/console) in your description goes to a 404.
Publicly displaying your error logs isn't a great idea. Attacker could use it as a sounding board.
Can't we send it to a log file on the system, so on-calls or authorized folks can investigate as needed?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] ChaiBapchya commented on issue #17597: move
stacktrance print to console rather than system.err
Posted by GitBox <gi...@apache.org>.
ChaiBapchya commented on issue #17597: move stacktrance print to console rather than system.err
URL: https://github.com/apache/incubator-mxnet/pull/17597#issuecomment-586492046
@anirudh2290 @access2rohit @marcoabreu
Since you reviewed the previous PR, thought you could take a look at this one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] marcoabreu commented on issue #17597: [CI] Move
stacktrace print to console rather than system.err
Posted by GitBox <gi...@apache.org>.
marcoabreu commented on issue #17597: [CI] Move stacktrace print to console rather than system.err
URL: https://github.com/apache/incubator-mxnet/pull/17597#issuecomment-587119287
The question is which security considerations have been put in place when the function was whitelisted
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] ChaiBapchya commented on issue #17597: [CI] Move
stacktrace print to console rather than system.err
Posted by GitBox <gi...@apache.org>.
ChaiBapchya commented on issue #17597: [CI] Move stacktrace print to console rather than system.err
URL: https://github.com/apache/incubator-mxnet/pull/17597#issuecomment-587110492
My point about security was a general point. I just made a note of it here.
This was function 'printStackTrace' was already present since #17065
I have only made 1 change (move that logging from std err to std out) - so that it gets print in the console.
Not sure what security consideration should I consider? This change of mine doesn't need any change in the existing Script Security (since it has already added printStackTrace to the approved function list)
It can be verified here - http://jenkins.mxnet-ci.amazon-ml.com/scriptApproval/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] marcoabreu commented on issue #17597: [CI] Move
stacktrace print to console rather than system.err
Posted by GitBox <gi...@apache.org>.
marcoabreu commented on issue #17597: [CI] Move stacktrace print to console rather than system.err
URL: https://github.com/apache/incubator-mxnet/pull/17597#issuecomment-587127491
Generally it's discouraged security wise to expose the stack trace of a system to the public since it offers insight into a protected system.
Exceptions are generally logged into a system log which is only accessible to administrators. This is also the case for Jenkins. Printing the stack trace of the Jenkins master to public though is not something that's recommended.
Printing the stack trace within docker is fine, within Jenkins though it's not.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] marcoabreu merged pull request #17597: [CI]
Remove print stacktrace (due to security concerns)
Posted by GitBox <gi...@apache.org>.
marcoabreu merged pull request #17597: [CI] Remove print stacktrace (due to security concerns)
URL: https://github.com/apache/incubator-mxnet/pull/17597
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] marcoabreu commented on issue #17597: [CI] Move
stacktrace print to console rather than system.err
Posted by GitBox <gi...@apache.org>.
marcoabreu commented on issue #17597: [CI] Move stacktrace print to console rather than system.err
URL: https://github.com/apache/incubator-mxnet/pull/17597#issuecomment-587658936
Jenkins already logs to the Jenkins error log which is only accessible by administrators. So I'd say there's no Todo except removing the security except.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] ChaiBapchya commented on issue #17597: [CI] Move
stacktrace print to console rather than system.err
Posted by GitBox <gi...@apache.org>.
ChaiBapchya commented on issue #17597: [CI] Move stacktrace print to console rather than system.err
URL: https://github.com/apache/incubator-mxnet/pull/17597#issuecomment-587766893
If I remove whitelist now before this gets merged, it will again block the jenkins build (due to that exception).
Hence to prevent Jenkins build face the exception, I'll remove it from whitelist once this gets merged. What do you reckon?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] ChaiBapchya commented on issue #17597: [CI] Move
stacktrace print to console rather than system.err
Posted by GitBox <gi...@apache.org>.
ChaiBapchya commented on issue #17597: [CI] Move stacktrace print to console rather than system.err
URL: https://github.com/apache/incubator-mxnet/pull/17597#issuecomment-587758491
1. I don't mind removing the print statement. Removed it.
2. It was introduced to aid developers to debug faster/easier (of course not at the cost of exposing security). I just wanted to verify if there's any way we can keep this functionality without hampering the security.
Anyway!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] ChaiBapchya commented on issue #17597: [CI] Move
stacktrace print to console rather than system.err
Posted by GitBox <gi...@apache.org>.
ChaiBapchya commented on issue #17597: [CI] Move stacktrace print to console rather than system.err
URL: https://github.com/apache/incubator-mxnet/pull/17597#issuecomment-587658470
Also #17065 introduced print stacktrace which got approved then by same folks.
In the meanwhile, I'm trying to collect what printStackTrace outputs on Jenkins console (if it leaks CI info in any way).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services