fix: add ping to Docker base image to fix Command Injection levels (#540)#541
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #541 +/- ##
============================================
+ Coverage 51.48% 51.52% +0.03%
Complexity 420 420
============================================
Files 65 65
Lines 2451 2467 +16
Branches 256 261 +5
============================================
+ Hits 1262 1271 +9
- Misses 1079 1082 +3
- Partials 110 114 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // docker build --target local -t vulnerableapp-base-local:latest -f Dockerfile.base . | ||
| from { | ||
| image = 'eclipse-temurin:17-jre-alpine' | ||
| image = 'docker://vulnerableapp-base-local:latest' |
There was a problem hiding this comment.
is there any other way than this. we don't want to manage docker image separately also jib is used in github workflows and if we start using docker file we need to do many changes in workflows as well.
is there a small docker image with ping functionality?
There was a problem hiding this comment.
Hi @preetkaran20, I tested all eclipse-temurin:17-jre variants (alpine, jammy, focal) , none include ping by default and Jib cannot run apt-get during build, so there's no way to use a base image without a separate Dockerfile.
As an alternative, I've updated the fix to handle this directly in CommandInjection.java, it now detects when ping is missing and returns a clear error message instead of a cryptic 'not found' error. This requires no Docker or workflow changes but only a single Java file is changed.
Let me know if you'd like a different approach!
There was a problem hiding this comment.
Then can you create a GitHub action to create the base image using your docker file and refer that base image in build.gradle.
Ensure creating base image that supports multiple architectures like current image does.
zeel2104
force-pushed
the
fix/command-injection-ping-540
branch
2 times, most recently
from
74fea99 to
56be25f
Compare
zeel2104
force-pushed
the
fix/command-injection-ping-540
branch
from
56be25f to
52e31d1
Compare
|
|
||
| RUN apk add --no-cache iputils | ||
|
|
||
| FROM eclipse-temurin:17-jre-jammy AS ci |
There was a problem hiding this comment.
do we need jammy? I think line 1 and 3 would be enough.
There was a problem hiding this comment.
i.e.
FROM eclipse-temurin:17-jre-alpine
RUN apk add --no-cache iputils
| } | ||
| } | ||
|
|
||
| def isLocalJibDockerBuild = |
There was a problem hiding this comment.
i think in all the cases we can just rely on base image in docker hub instead of local builds. I think you added this to make testing easier for baseclass but i think we are just adding iputils package so we don't need this.
| image = 'eclipse-temurin:17-jre-alpine' | ||
| // Local builds still use a Docker daemon image so contributors can iterate | ||
| // without publishing anything first. | ||
| image = isLocalJibDockerBuild |
There was a problem hiding this comment.
always choose the built image name that we will build using docker.yml
| ./gradlew jib \ | ||
| -Djib.to.image=sasanlabs/owasp-vulnerableapp:unreleased \ | ||
| -Djib.to.tags=$GITVERSION_SEMVER,latest | ||
| -Pvulnerableapp.base.image=${BASE_IMAGE}:${{ steps.gitversion.outputs.semver }} \ |
There was a problem hiding this comment.
i think if we update build.gradle, we can remove this part and make it simple.
|
Updated this based on the review feedback:
I also reran the local Gradle build and it passed. |
preetkaran20
left a comment
preetkaran20
left a comment
There was a problem hiding this comment.
Looks good to me. Thank you. Merging the PR now.
3 participants
Fixes #540