Skip to content

Commit 8b44e4f

Browse files
mhoffrogmichael-o
mhoffrog
authored and
michael-o
committed
[SCM-1028] Fix clear password logging vulnerability
- GitUtil.java: - add method maskPasswordInUrl(String urlWithCredentials) - implementation taken from AnonymousCommandLine.java - improve regex pattern to be more precise - replace wrapped with delimiters ':' and '@' to avoid replacing the password within probable other places of the URL to avoid password guessing by using e.g. redundant URL parameters - AnonymousCommandLine.java: - move current password masking implementation to GitUtil - use implementation from GitUtil - GitScmProviderRepository.java: - add method getFetchUrlWithMaskedPassword() - add method getPushUrlWithMaskedPassword() - toString(): - BREAKING change: provide URL content with masked password to reduce risk of usage within logs or exceptions with showing passwords by that - JGitUtils.java: - method prepareSession(Git git, GitScmProviderRepository repository): - log using methods: - GitScmProviderRepository.getFetchUrlWithMaskedPassword() - GitScmProviderRepository.getPushUrlWithMaskedPassword() - GitRemoteInfoCommand.java: - use GitScmProviderRepository.getFetchUrlWithMaskedPassword() for exception message - Update JUnit tests accordingly: - GitScmProviderRepositoryTest.java - GitCommandLineUtilsTest.java This closes #237
1 parent d24c832 commit 8b44e4f

File tree

7 files changed

+75
-52
lines changed

7 files changed

+75
-52
lines changed

‎maven-scm-providers/maven-scm-providers-git/maven-scm-provider-git-commons/src/main/java/org/apache/maven/scm/provider/git/repository/GitScmProviderRepository.java‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import org.apache.maven.scm.ScmException;
2727
import org.apache.maven.scm.provider.ScmProviderRepository;
2828
import org.apache.maven.scm.provider.ScmProviderRepositoryWithHost;
29+
import org.apache.maven.scm.provider.git.util.GitUtil;
2930

3031
/**
3132
* @author <a href="mailto:[email protected]">Emmanuel Venisse</a>
@@ -185,13 +186,27 @@ public String getFetchUrl() {
185186
return getUrl(fetchInfo);
186187
}
187188

189+
/**
190+
* @return the URL to fetch from with masked password used for logging purposes only
191+
*/
192+
public String getFetchUrlWithMaskedPassword() {
193+
return GitUtil.maskPasswordInUrl(getFetchUrl());
194+
}
195+
188196
/**
189197
* @return the URL used to push to the upstream repository
190198
*/
191199
public String getPushUrl() {
192200
return getUrl(pushInfo);
193201
}
194202

203+
/**
204+
* @return the URL to push to with masked password used for logging purposes only
205+
*/
206+
public String getPushUrlWithMaskedPassword() {
207+
return GitUtil.maskPasswordInUrl(getPushUrl());
208+
}
209+
195210
/**
196211
* Parse the given url string and store all the extracted
197212
* information in a {@code RepositoryUrl}
@@ -385,6 +400,7 @@ private String parseHostAndPort(RepositoryUrl repoUrl, String url) throws ScmExc
385400
/**
386401
* {@inheritDoc}
387402
*/
403+
@Override
388404
public String getRelativePath(ScmProviderRepository ancestor) {
389405
if (ancestor instanceof GitScmProviderRepository) {
390406
GitScmProviderRepository gitAncestor = (GitScmProviderRepository) ancestor;
@@ -403,11 +419,15 @@ public String getRelativePath(ScmProviderRepository ancestor) {
403419
/**
404420
* {@inheritDoc}
405421
*/
422+
@Override
406423
public String toString() {
407424
// yes we really like to check if those are the exact same instance!
408425
if (fetchInfo == pushInfo) {
409-
return getUrl(fetchInfo);
426+
return getFetchUrlWithMaskedPassword();
410427
}
411-
return URL_DELIMITER_FETCH + getUrl(fetchInfo) + URL_DELIMITER_PUSH + getUrl(pushInfo);
428+
return URL_DELIMITER_FETCH
429+
+ getFetchUrlWithMaskedPassword()
430+
+ URL_DELIMITER_PUSH
431+
+ getPushUrlWithMaskedPassword();
412432
}
413433
}

‎maven-scm-providers/maven-scm-providers-git/maven-scm-provider-git-commons/src/main/java/org/apache/maven/scm/provider/git/util/GitUtil.java‎

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,21 +20,30 @@
2020

2121
import java.io.File;
2222
import java.io.IOException;
23+
import java.util.regex.Matcher;
24+
import java.util.regex.Pattern;
2325

26+
import org.apache.commons.lang3.StringUtils;
2427
import org.apache.maven.scm.providers.gitlib.settings.Settings;
2528
import org.apache.maven.scm.providers.gitlib.settings.io.xpp3.GitXpp3Reader;
2629
import org.codehaus.plexus.util.ReaderFactory;
2730
import org.codehaus.plexus.util.xml.pull.XmlPullParserException;
2831

2932
/**
3033
* @author <a href="mailto:[email protected]">Emmanuel Venisse</a>
31-
*
3234
*/
3335
public class GitUtil {
3436
protected static final String GIT_SETTINGS_FILENAME = "git-settings.xml";
3537

3638
public static final File DEFAULT_SETTINGS_DIRECTORY = new File(System.getProperty("user.home"), ".scm");
3739

40+
/** The password placeholder must contain delimiters.
41+
* Otherwise replacing may replace other portions of the URL as well
42+
* and in worst case passwords could be guessed. */
43+
public static final String PASSWORD_PLACE_HOLDER_WITH_DELIMITERS = ":********@";
44+
45+
private static final Pattern PASSWORD_IN_URL_PATTERN = Pattern.compile("^.*(:[^/].*@).*$");
46+
3847
private static File settingsDirectory = DEFAULT_SETTINGS_DIRECTORY;
3948

4049
private static Settings settings;
@@ -77,4 +86,24 @@ public static void setSettingsDirectory(File directory) {
7786
public static File getSettingsFile() {
7887
return new File(settingsDirectory, GIT_SETTINGS_FILENAME);
7988
}
89+
90+
/**
91+
* Provides an anonymous output to mask password. Considering URL of type :
92+
* &lt;&lt;protocol&gt;&gt;://&lt;&lt;user&gt;&gt;:&lt;&lt;password&gt;&gt;@
93+
* &lt;&lt;host_definition&gt;&gt;
94+
*
95+
* @param urlWithCredentials
96+
* @return urlWithCredentials but password masked with stars
97+
*/
98+
public static String maskPasswordInUrl(String urlWithCredentials) {
99+
String output = urlWithCredentials;
100+
final Matcher passwordMatcher = PASSWORD_IN_URL_PATTERN.matcher(output);
101+
if (passwordMatcher.find()) {
102+
// clear password with delimiters
103+
final String clearPasswordWithDelimiters = passwordMatcher.group(1);
104+
// to be replaced in output by stars with delimiters
105+
output = StringUtils.replace(output, clearPasswordWithDelimiters, PASSWORD_PLACE_HOLDER_WITH_DELIMITERS);
106+
}
107+
return output;
108+
}
80109
}

‎maven-scm-providers/maven-scm-providers-git/maven-scm-provider-git-commons/src/test/java/org/apache/maven/scm/provider/git/repository/GitScmProviderRepositoryTest.java‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ public void testLegalHttpURLWithUser() throws Exception {
126126
public void testLegalHttpURLWithUserPassword() throws Exception {
127127
testUrl(
128128
"scm:git:http://user:[email protected]",
129-
null,
129+
"http://user:********@gitrepos.apache.org",
130130
"http://user:[email protected]",
131131
null,
132132
"user",
@@ -174,7 +174,7 @@ public void testLegalHttpsURLWithUser() throws Exception {
174174
public void testLegalHttpsURLWithUserPassword() throws Exception {
175175
testUrl(
176176
"scm:git:https://user:[email protected]",
177-
null,
177+
"https://user:********@gitrepos.apache.org",
178178
"https://user:[email protected]",
179179
null,
180180
"user",
@@ -202,7 +202,7 @@ public void testLegalSshURLWithUser() throws Exception {
202202
public void testLegalSshURLWithUserPassword() throws Exception {
203203
testUrl(
204204
"scm:git:ssh://user:[email protected]",
205-
null,
205+
"ssh://user:********@gitrepos.apache.org",
206206
"ssh://user:[email protected]",
207207
null,
208208
"user",
@@ -289,12 +289,12 @@ public void testGitDevUrlWithNumberedRepoAndMinus() throws Exception, ScmReposit
289289
public void testSpecialCharacters() throws Exception {
290290
testUrl(
291291
"scm:git:http://gitrepos.apache.org",
292-
"@_&_:_?_#_%20",
292+
"/_@_&_:_?_#_%20",
293293
"pass word",
294294
null,
295295
"http://gitrepos.apache.org",
296296
null,
297-
"http://%40_&_:_%3F_%23_%2520:pass%[email protected]",
297+
"http://%2F_%40_&_:_%3F_%23_%2520:pass%[email protected]",
298298
null,
299299
"gitrepos.apache.org",
300300
0,
@@ -303,11 +303,11 @@ public void testSpecialCharacters() throws Exception {
303303
testUrl(
304304
"scm:git:http://gitrepos.apache.org",
305305
"user name",
306-
"@_&_:_?_#_%20",
306+
"/_@_&_:_?_#_%20",
307307
null,
308308
"http://gitrepos.apache.org",
309309
null,
310-
"http://user%20name:%40_&_:_%3F_%23_%[email protected]",
310+
"http://user%20name:%2F_%40_&_:_%3F_%23_%[email protected]",
311311
null,
312312
"gitrepos.apache.org",
313313
0,
@@ -362,7 +362,7 @@ public void testLegalGitPortUrl() throws Exception {
362362

363363
testUrl(
364364
"scm:git:ssh://username:[email protected]/pmgt/trunk",
365-
null,
365+
"ssh://username:********@gitrepos.apache.org/pmgt/trunk",
366366
"ssh://username:[email protected]/pmgt/trunk",
367367
null,
368368
"username",
@@ -376,7 +376,7 @@ public void testLegalGitPortUrl() throws Exception {
376376
public void testUsernameWithAtAndPasswordInUrl() throws ScmRepositoryException, Exception {
377377
testUrl(
378378
"scm:git:http://[email protected]:[email protected]:8800/pmgt/trunk",
379-
null,
379+
"http://username%40site.com:********@gitrepos.apache.org:8800/pmgt/trunk",
380380
"http://username%40site.com:[email protected]:8800/pmgt/trunk",
381381
null,
382382
"[email protected]",
@@ -394,7 +394,7 @@ public void testUsernameWithAtAndPasswordInUrl() throws ScmRepositoryException,
394394
public void testHttpFetchSshPushUrl() throws Exception {
395395
testUrl(
396396
"scm:git:[fetch=]http://git.apache.org/myprj.git[push=]ssh://myuser:[email protected]/~/myrepo/myprj.git",
397-
"[fetch=]http://myuser:mypassword@git.apache.org/myprj.git[push=]ssh://myuser:mypassword@git.apache.org/~/myrepo/myprj.git",
397+
"[fetch=]http://myuser:********@git.apache.org/myprj.git[push=]ssh://myuser:********@git.apache.org/~/myrepo/myprj.git",
398398
"http://myuser:[email protected]/myprj.git",
399399
"ssh://myuser:[email protected]/~/myrepo/myprj.git",
400400
"myuser",
@@ -405,7 +405,7 @@ public void testHttpFetchSshPushUrl() throws Exception {
405405

406406
testUrl(
407407
"scm:git:[push=]ssh://myuser:[email protected]/~/myrepo/myprj.git[fetch=]http://git.apache.org/myprj.git",
408-
"[fetch=]http://myuser:mypassword@git.apache.org/myprj.git[push=]ssh://myuser:mypassword@git.apache.org/~/myrepo/myprj.git",
408+
"[fetch=]http://myuser:********@git.apache.org/myprj.git[push=]ssh://myuser:********@git.apache.org/~/myrepo/myprj.git",
409409
"http://myuser:[email protected]/myprj.git",
410410
"ssh://myuser:[email protected]/~/myrepo/myprj.git",
411411
"myuser",

‎maven-scm-providers/maven-scm-providers-git/maven-scm-provider-gitexe/src/main/java/org/apache/maven/scm/provider/git/gitexe/command/AnonymousCommandLine.java‎

Lines changed: 2 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,7 @@
1818
*/
1919
package org.apache.maven.scm.provider.git.gitexe.command;
2020

21-
import java.util.regex.Matcher;
22-
import java.util.regex.Pattern;
23-
21+
import org.apache.maven.scm.provider.git.util.GitUtil;
2422
import org.codehaus.plexus.util.cli.Commandline;
2523

2624
/**
@@ -29,25 +27,14 @@
2927
*/
3028
public class AnonymousCommandLine extends Commandline {
3129

32-
public static final String PASSWORD_PLACE_HOLDER = "********";
33-
34-
private Pattern passwordPattern = Pattern.compile("^.*:(.*)@.*$");
35-
3630
/**
3731
* Provides an anonymous output to mask password. Considering URL of type :
3832
* &lt;&lt;protocol&gt;&gt;://&lt;&lt;user&gt;&gt;:&lt;&lt;password&gt;&gt;@
3933
* &lt;&lt;host_definition&gt;&gt;
4034
*/
4135
@Override
4236
public String toString() {
43-
String output = super.toString();
44-
final Matcher passwordMatcher = passwordPattern.matcher(output);
45-
if (passwordMatcher.find()) {
46-
// clear password
47-
final String clearPassword = passwordMatcher.group(1);
48-
// to be replaced in output by stars
49-
output = output.replace(clearPassword, PASSWORD_PLACE_HOLDER);
50-
}
37+
String output = GitUtil.maskPasswordInUrl(super.toString());
5138
return output;
5239
}
5340
}

‎maven-scm-providers/maven-scm-providers-git/maven-scm-provider-gitexe/src/main/java/org/apache/maven/scm/provider/git/gitexe/command/remoteinfo/GitRemoteInfoCommand.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ public RemoteInfoScmResult executeRemoteInfoCommand(
5656

5757
int exitCode = GitCommandLineUtils.execute(clLsRemote, consumer, stderr);
5858
if (exitCode != 0) {
59-
throw new ScmException("unable to execute ls-remote on " + gitRepository.getFetchUrl());
59+
throw new ScmException("unable to execute ls-remote on " + gitRepository.getFetchUrlWithMaskedPassword());
6060
}
6161

6262
return consumer.getRemoteInfoScmResult();

‎maven-scm-providers/maven-scm-providers-git/maven-scm-provider-gitexe/src/test/java/org/apache/maven/scm/provider/git/gitexe/command/GitCommandLineUtilsTest.java‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828

2929
import org.apache.maven.scm.ScmException;
3030
import org.apache.maven.scm.provider.git.repository.GitScmProviderRepository;
31+
import org.apache.maven.scm.provider.git.util.GitUtil;
3132
import org.codehaus.plexus.util.Os;
3233
import org.codehaus.plexus.util.cli.Commandline;
3334
import org.junit.Test;
@@ -91,13 +92,13 @@ public void testPasswordAnonymous() throws Exception {
9192
assertFalse(
9293
MessageFormat.format(
9394
"The target log message should not contain <{0}> but it contains <{1}>",
94-
AnonymousCommandLine.PASSWORD_PLACE_HOLDER, commandLineArgs[i]),
95-
commandLineArgs[i].contains(AnonymousCommandLine.PASSWORD_PLACE_HOLDER));
95+
GitUtil.PASSWORD_PLACE_HOLDER_WITH_DELIMITERS, commandLineArgs[i]),
96+
commandLineArgs[i].contains(GitUtil.PASSWORD_PLACE_HOLDER_WITH_DELIMITERS));
9697
}
9798

98-
final String scmUrlFakeForTest = "https://user:"
99-
.concat(AnonymousCommandLine.PASSWORD_PLACE_HOLDER)
100-
.concat("@foo.com/git/trunk");
99+
final String scmUrlFakeForTest = "https://user"
100+
.concat(GitUtil.PASSWORD_PLACE_HOLDER_WITH_DELIMITERS)
101+
.concat("foo.com/git/trunk");
101102

102103
assertTrue(
103104
MessageFormat.format(

‎maven-scm-providers/maven-scm-providers-git/maven-scm-provider-jgit/src/main/java/org/apache/maven/scm/provider/git/jgit/command/JGitUtils.java‎

Lines changed: 3 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,6 @@
2020

2121
import java.io.File;
2222
import java.io.IOException;
23-
import java.io.UnsupportedEncodingException;
24-
import java.net.URLEncoder;
2523
import java.util.ArrayList;
2624
import java.util.Collection;
2725
import java.util.Date;
@@ -132,28 +130,16 @@ public static ProgressMonitor getMonitor() {
132130
* @param git the instance to configure (only in memory, not saved)
133131
* @param repository the repo config to be used
134132
* @return {@link CredentialsProvider} in case there are credentials
135-
* informations configured in the repository.
133+
* informations configured in the repository.
136134
*/
137135
public static CredentialsProvider prepareSession(Git git, GitScmProviderRepository repository) {
138136
StoredConfig config = git.getRepository().getConfig();
139137
config.setString("remote", "origin", "url", repository.getFetchUrl());
140138
config.setString("remote", "origin", "pushURL", repository.getPushUrl());
141139

142140
// make sure we do not log any passwords to the output
143-
String password = StringUtils.isNotBlank(repository.getPassword())
144-
? repository.getPassword().trim()
145-
: "no-pwd-defined";
146-
// if password contains special characters it won't match below.
147-
// Try encoding before match. (Passwords without will be unaffected)
148-
try {
149-
password = URLEncoder.encode(password, "UTF-8");
150-
} catch (UnsupportedEncodingException e) {
151-
// UTF-8 should be valid
152-
// TODO use a logger
153-
System.out.println("Ignore UnsupportedEncodingException when trying to encode password");
154-
}
155-
LOGGER.info("fetch url: " + repository.getFetchUrl().replace(password, "******"));
156-
LOGGER.info("push url: " + repository.getPushUrl().replace(password, "******"));
141+
LOGGER.info("fetch url: " + repository.getFetchUrlWithMaskedPassword());
142+
LOGGER.info("push url: " + repository.getPushUrlWithMaskedPassword());
157143
return getCredentials(repository);
158144
}
159145

0 commit comments

Comments
 (0)