Skip to content

Commit 041214b

Browse files
daniel-mohedanodaniel-mohedano
authored
Add validation checks for user provided arguments in git commands (#9092)
1 parent 796bf33 commit 041214b

File tree

4 files changed

+233
-11
lines changed

4 files changed

+233
-11
lines changed

‎dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/ShellGitClient.java‎

Lines changed: 41 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
import datadog.trace.api.civisibility.telemetry.CiVisibilityDistributionMetric;
77
import datadog.trace.api.civisibility.telemetry.CiVisibilityMetricCollector;
88
import datadog.trace.api.civisibility.telemetry.tag.Command;
9+
import datadog.trace.api.git.GitUtils;
910
import datadog.trace.civisibility.diff.LineDiff;
1011
import datadog.trace.civisibility.utils.ShellCommandExecutor;
1112
import datadog.trace.util.Strings;
@@ -149,13 +150,13 @@ public void unshallow(@Nullable String remoteCommitReference)
149150
.trim();
150151

151152
// refetch data from the server for the given period of time
152-
if (remoteCommitReference != null) {
153+
if (remoteCommitReference != null && GitUtils.isValidRef(remoteCommitReference)) {
153154
String headSha = getSha(remoteCommitReference);
154155
commandExecutor.executeCommand(
155156
ShellCommandExecutor.OutputParser.IGNORE,
156157
"git",
157158
"fetch",
158-
String.format("--shallow-since=='%s'", latestCommitsSince),
159+
String.format("--shallow-since='%s'", latestCommitsSince),
159160
"--update-shallow",
160161
"--filter=blob:none",
161162
"--recurse-submodules=no",
@@ -166,7 +167,7 @@ public void unshallow(@Nullable String remoteCommitReference)
166167
ShellCommandExecutor.OutputParser.IGNORE,
167168
"git",
168169
"fetch",
169-
String.format("--shallow-since=='%s'", latestCommitsSince),
170+
String.format("--shallow-since='%s'", latestCommitsSince),
170171
"--update-shallow",
171172
"--filter=blob:none",
172173
"--recurse-submodules=no",
@@ -231,6 +232,9 @@ public String getRepoRoot() throws IOException, TimeoutException, InterruptedExc
231232
@Override
232233
public String getRemoteUrl(String remoteName)
233234
throws IOException, TimeoutException, InterruptedException {
235+
if (!GitUtils.isValidRef(remoteName)) {
236+
return null;
237+
}
234238
return executeCommand(
235239
Command.GET_REPOSITORY,
236240
() ->
@@ -274,6 +278,9 @@ public String getCurrentBranch() throws IOException, TimeoutException, Interrupt
274278
@Override
275279
public List<String> getTags(String commit)
276280
throws IOException, TimeoutException, InterruptedException {
281+
if (GitUtils.isNotValidCommit(commit)) {
282+
return Collections.emptyList();
283+
}
277284
return executeCommand(
278285
Command.OTHER,
279286
() -> {
@@ -302,6 +309,9 @@ public List<String> getTags(String commit)
302309
@Override
303310
public String getSha(String reference)
304311
throws IOException, TimeoutException, InterruptedException {
312+
if (GitUtils.isNotValidCommit(reference)) {
313+
return null;
314+
}
305315
return executeCommand(
306316
Command.OTHER,
307317
() ->
@@ -324,6 +334,9 @@ public String getSha(String reference)
324334
@Override
325335
public String getFullMessage(String commit)
326336
throws IOException, TimeoutException, InterruptedException {
337+
if (GitUtils.isNotValidCommit(commit)) {
338+
return null;
339+
}
327340
return executeCommand(
328341
Command.OTHER,
329342
() ->
@@ -346,6 +359,9 @@ public String getFullMessage(String commit)
346359
@Override
347360
public String getAuthorName(String commit)
348361
throws IOException, TimeoutException, InterruptedException {
362+
if (GitUtils.isNotValidCommit(commit)) {
363+
return null;
364+
}
349365
return executeCommand(
350366
Command.OTHER,
351367
() ->
@@ -368,6 +384,9 @@ public String getAuthorName(String commit)
368384
@Override
369385
public String getAuthorEmail(String commit)
370386
throws IOException, TimeoutException, InterruptedException {
387+
if (GitUtils.isNotValidCommit(commit)) {
388+
return null;
389+
}
371390
return executeCommand(
372391
Command.OTHER,
373392
() ->
@@ -390,6 +409,9 @@ public String getAuthorEmail(String commit)
390409
@Override
391410
public String getAuthorDate(String commit)
392411
throws IOException, TimeoutException, InterruptedException {
412+
if (GitUtils.isNotValidCommit(commit)) {
413+
return null;
414+
}
393415
return executeCommand(
394416
Command.OTHER,
395417
() ->
@@ -412,6 +434,9 @@ public String getAuthorDate(String commit)
412434
@Override
413435
public String getCommitterName(String commit)
414436
throws IOException, TimeoutException, InterruptedException {
437+
if (GitUtils.isNotValidCommit(commit)) {
438+
return null;
439+
}
415440
return executeCommand(
416441
Command.OTHER,
417442
() ->
@@ -434,6 +459,9 @@ public String getCommitterName(String commit)
434459
@Override
435460
public String getCommitterEmail(String commit)
436461
throws IOException, TimeoutException, InterruptedException {
462+
if (GitUtils.isNotValidCommit(commit)) {
463+
return null;
464+
}
437465
return executeCommand(
438466
Command.OTHER,
439467
() ->
@@ -456,6 +484,9 @@ public String getCommitterEmail(String commit)
456484
@Override
457485
public String getCommitterDate(String commit)
458486
throws IOException, TimeoutException, InterruptedException {
487+
if (GitUtils.isNotValidCommit(commit)) {
488+
return null;
489+
}
459490
return executeCommand(
460491
Command.OTHER,
461492
() ->
@@ -601,6 +632,10 @@ private Path createTempDirectory() throws IOException {
601632
public String getBaseCommitSha(
602633
@Nullable String baseBranch, @Nullable String settingsDefaultBranch)
603634
throws IOException, TimeoutException, InterruptedException {
635+
if ((baseBranch != null && !GitUtils.isValidRef(baseBranch))
636+
|| (settingsDefaultBranch != null && !GitUtils.isValidRef(settingsDefaultBranch))) {
637+
return null;
638+
}
604639
return executeCommand(
605640
Command.BASE_COMMIT_SHA,
606641
() -> {
@@ -956,10 +991,10 @@ String getMergeBase(String baseBranch, String sourceBranch)
956991
@Override
957992
public LineDiff getGitDiff(String baseCommit, String targetCommit)
958993
throws IOException, TimeoutException, InterruptedException {
959-
if (Strings.isBlank(baseCommit)) {
994+
if (Strings.isBlank(baseCommit) || !GitUtils.isValidCommitSha(baseCommit)) {
960995
LOGGER.debug("Base commit info is not available, returning empty git diff");
961996
return null;
962-
} else if (Strings.isNotBlank(targetCommit)) {
997+
} else if (Strings.isNotBlank(targetCommit) && GitUtils.isValidCommitSha(targetCommit)) {
963998
return executeCommand(
964999
Command.DIFF,
9651000
() ->
@@ -1041,7 +1076,7 @@ public Factory(Config config, CiVisibilityMetricCollector metricCollector) {
10411076
@Override
10421077
public GitClient create(@Nullable String repoRoot) {
10431078
long commandTimeoutMillis = config.getCiVisibilityGitCommandTimeoutMillis();
1044-
if (repoRoot != null) {
1079+
if (repoRoot != null && GitUtils.isValidPath(repoRoot)) {
10451080
ShellGitClient client =
10461081
new ShellGitClient(
10471082
metricCollector, repoRoot, "1 month ago", 1000, commandTimeoutMillis);

‎internal-api/src/main/java/datadog/trace/api/git/GitUtils.java‎

Lines changed: 75 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,16 @@
33
import static datadog.trace.api.git.RawParseUtils.decode;
44
import static datadog.trace.api.git.RawParseUtils.nextLF;
55

6+
import datadog.trace.util.Strings;
67
import java.io.ByteArrayOutputStream;
78
import java.io.IOException;
89
import java.net.URI;
910
import java.nio.charset.StandardCharsets;
1011
import java.util.regex.Pattern;
1112
import java.util.zip.DataFormatException;
1213
import java.util.zip.Inflater;
14+
import javax.annotation.Nonnull;
15+
import javax.annotation.Nullable;
1316
import org.slf4j.Logger;
1417
import org.slf4j.LoggerFactory;
1518

@@ -19,6 +22,26 @@ public class GitUtils {
1922
private static final Pattern REFS_HEADS_PATTERN = Pattern.compile("refs/heads/", Pattern.LITERAL);
2023
private static final Pattern REFS_TAGS_PATTERN = Pattern.compile("refs/tags/", Pattern.LITERAL);
2124
private static final Pattern TAGS_PATTERN = Pattern.compile("tags/", Pattern.LITERAL);
25+
// Based on https://git-scm.com/docs/git-check-ref-format
26+
private static final Pattern INVALID_REF_PATTERN =
27+
Pattern.compile(
28+
"^/" // starts with /
29+
+ "|//" // contains //
30+
+ "|\\.\\." // contains ..
31+
+ "|@\\{" // contains @{
32+
+ "|\\.$" // ends with a dot
33+
+ "|\\.lock(/|$)" // ends with .lock in any path component
34+
+ "|(?:^|/)\\." // any component starts with a dot
35+
+ "|\\s" // contains space
36+
+ "|[~^:?*\\[\\\\]" // contains ~ ^ : ? * [ \
37+
+ "|^@$" // is only @
38+
+ "|/$" // ends with slash
39+
);
40+
private static final Pattern PATH_PATTERN = Pattern.compile("^[a-zA-Z0-9_./-]+$");
41+
private static final Pattern SHELL_METACHAR_PATTERN = Pattern.compile(".*[`$&|;<>\n\r#].*");
42+
43+
private static final int SHORT_SHA_LENGTH = 7;
44+
private static final int FULL_SHA_LENGTH = 40;
2245

2346
private static final Logger log = LoggerFactory.getLogger(GitUtils.class);
2447

@@ -205,12 +228,28 @@ private static void logErrorInflating(final String reason) {
205228
* Checks if the provided string is a valid commit SHA:
206229
*
207230
* <ul>
208-
* <li>length >= 40
231+
* <li>length >= 7
209232
* <li>every character is a hexadecimal digit
210233
* </ul>
211234
*/
212235
public static boolean isValidCommitSha(final String commitSha) {
213-
if (commitSha == null || commitSha.length() < 40) {
236+
return isValidCommitSha(commitSha, SHORT_SHA_LENGTH);
237+
}
238+
239+
/**
240+
* Checks if the provided string is a valid commit SHA of full length:
241+
*
242+
* <ul>
243+
* <li>length >= 40
244+
* <li>every character is a hexadecimal digit
245+
* </ul>
246+
*/
247+
public static boolean isValidCommitShaFull(final String commitSha) {
248+
return isValidCommitSha(commitSha, FULL_SHA_LENGTH);
249+
}
250+
251+
private static boolean isValidCommitSha(final String commitSha, final int minLength) {
252+
if (commitSha == null || commitSha.length() < minLength) {
214253
return false;
215254
}
216255
for (char c : commitSha.toCharArray()) {
@@ -224,4 +263,38 @@ public static boolean isValidCommitSha(final String commitSha) {
224263
private static boolean isHex(char c) {
225264
return (c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F');
226265
}
266+
267+
/** Checks if a string that will be used as a Git command argument is valid and safe */
268+
static boolean isValidArg(@Nullable String arg) {
269+
if (Strings.isBlank(arg)) {
270+
return false;
271+
}
272+
return !SHELL_METACHAR_PATTERN.matcher(arg).find();
273+
}
274+
275+
/** Checks if the provided string is a valid Git reference (branch, tag, etc.) */
276+
public static boolean isValidRef(@Nullable String ref) {
277+
if (Strings.isBlank(ref)) {
278+
return false;
279+
}
280+
281+
for (char c : ref.toCharArray()) {
282+
if (c < 32 || c == 127) return false;
283+
}
284+
285+
if (INVALID_REF_PATTERN.matcher(ref).find()) {
286+
return false;
287+
}
288+
return isValidArg(ref);
289+
}
290+
291+
/** Checks if the provided string is a valid system path for Git operations */
292+
public static boolean isValidPath(@Nonnull String path) {
293+
return PATH_PATTERN.matcher(path).matches();
294+
}
295+
296+
/** Checks if the provided string is neither a valid commit SHA nor a valid Git reference */
297+
public static boolean isNotValidCommit(@Nullable String commit) {
298+
return !isValidCommitSha(commit) && !isValidRef(commit);
299+
}
227300
}

‎internal-api/src/main/java/datadog/trace/api/git/UserSuppliedGitInfoBuilder.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ public GitInfo build(@Nullable String repositoryPath) {
8989
}
9090

9191
String commitSha = gitInfo.getCommit().getSha();
92-
if (!GitUtils.isValidCommitSha(commitSha)) {
92+
if (!GitUtils.isValidCommitShaFull(commitSha)) {
9393
log.error(
9494
"Git commit SHA could not be resolved or is invalid: "
9595
+ commitSha

0 commit comments

Comments
 (0)