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

Fix/captcha repeater vulnerability #427

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
a503bb4
Added captcha generation using nano captcha library. Added initial va…
ivanmrsulja Nov 20, 2023
461213d
Added variable challenge length.
ivanmrsulja Nov 20, 2023
d06b400
Added Google reCaptcha. Added simple configuration feature toggle.
ivanmrsulja Nov 21, 2023
58bdd92
Added java docs.
ivanmrsulja Nov 22, 2023
ca18759
Fixed log4J version bug. Added unit tests for captcha functionality.
ivanmrsulja Nov 22, 2023
fe87586
Added guava cache for mitigation of DoS by generating challenges.
ivanmrsulja Nov 22, 2023
262b99a
Aligned sl4j-api dependency version.
ivanmrsulja Nov 23, 2023
83c553b
Added refresh button for default challenge.
ivanmrsulja Nov 28, 2023
9156eba
Improved error messages and added localization.
ivanmrsulja Nov 29, 2023
0655072
Update home/src/main/resources/rdf/i18n/de_DE/interface-i18n/firsttim…
ivanmrsulja Dec 5, 2023
c5d6573
Update home/src/main/resources/rdf/i18n/pt_BR/interface-i18n/firsttim…
ivanmrsulja Dec 5, 2023
d616c95
Update home/src/main/resources/rdf/i18n/pt_BR/interface-i18n/firsttim…
ivanmrsulja Dec 5, 2023
3f86241
Added missing licence header. Refactored code to avoid passing vreq w…
ivanmrsulja Dec 11, 2023
b9d7620
Update home/src/main/resources/rdf/i18n/ru_RU/interface-i18n/firsttim…
ivanmrsulja Dec 11, 2023
3060975
Update home/src/main/resources/rdf/i18n/ru_RU/interface-i18n/firsttim…
ivanmrsulja Dec 11, 2023
bdc9820
Update home/src/main/resources/rdf/i18n/ru_RU/interface-i18n/firsttim…
ivanmrsulja Dec 11, 2023
187dc85
Update home/src/main/resources/rdf/i18n/ru_RU/interface-i18n/firsttim…
ivanmrsulja Dec 11, 2023
c195f60
Switched to using getInstance() instead of deprecated getBean() method.
ivanmrsulja Dec 11, 2023
4233deb
Fixed frontend display error.
ivanmrsulja Dec 12, 2023
34842c7
Added captcha feature toggle with difficulty setting.
ivanmrsulja Dec 15, 2023
358e8ae
Updated captcha configuration.
ivanmrsulja Dec 15, 2023
2e7b041
Made nanocaptcha challenge little bigger.
ivanmrsulja Dec 15, 2023
dee609f
Improved spanish localization.
ivanmrsulja Dec 15, 2023
71dbdfd
Made configuration options as enumertions.
ivanmrsulja Dec 18, 2023
0aa5ff7
Made configuration case insensitive, updated configuration docs.
ivanmrsulja Dec 19, 2023
af1ddae
Improved french localization.
ivanmrsulja Dec 19, 2023
aef5f86
Refactored code to use provider pattern.
ivanmrsulja Dec 22, 2023
74c20c1
Updated javadocs.
ivanmrsulja Dec 22, 2023
fc92fc3
Update home/src/main/resources/config/example.runtime.properties
ivanmrsulja Jan 9, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions api/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,21 @@
<artifactId>argon2-jvm</artifactId>
<version>2.11</version>
</dependency>
<dependency>
<groupId>net.logicsquad</groupId>
<artifactId>nanocaptcha</artifactId>
<version>1.5</version>
</dependency>
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
<version>1.7.36</version>
</dependency>
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
<version>30.1-jre</version>
</dependency>
<dependency>
<groupId>org.apache.httpcomponents</groupId>
<artifactId>fluent-hc</artifactId>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package edu.cornell.mannlib.vitro.webapp.beans;

import java.io.IOException;
import java.util.Map;

import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;

/**
* The AbstractCaptchaProvider is an abstract class providing a base structure for captcha providers.
* It includes methods for generating refresh challenges, adding captcha-related fields to page context,
* and validating user inputs against captcha challenges.
*
* @see CaptchaBundle
*/
public abstract class AbstractCaptchaProvider {

protected static final Log log = LogFactory.getLog(AbstractCaptchaProvider.class.getName());

/**
* Generates a refresh challenge, typically used for updating the captcha displayed on the page.
* Returns empty CaptchaBundle in case of 3rd party implementations
*
* @return CaptchaBundle containing the refreshed captcha challenge.
* @throws IOException If there is an issue generating the refresh challenge.
*/
abstract CaptchaBundle generateRefreshChallenge() throws IOException;

/**
* Adds captcha-related fields to the provided page context, allowing integration with web pages.
*
* @param context The context map representing the page's variables.
* @throws IOException If there is an issue adding captcha-related fields to the page context.
*/
abstract void addCaptchaRelatedFieldsToPageContext(Map<String, Object> context) throws IOException;

/**
* Validates the user input against a captcha challenge identified by the provided challengeId.
*
* @param captchaInput The user's input to be validated.
* @param challengeId The identifier of the captcha challenge (ignored in case of 3rd party implementations).
* @return True if the input is valid, false otherwise.
*/
boolean validateCaptcha(String captchaInput, String challengeId) {
return false;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/* $This file is distributed under the terms of the license in LICENSE$ */

package edu.cornell.mannlib.vitro.webapp.beans;
ivanmrsulja marked this conversation as resolved.
Show resolved Hide resolved

import java.util.Objects;

/**
* Represents a bundle containing a CAPTCHA image in Base64 format, the associated code,
* and a unique challenge identifier.
*
* @author Ivan Mrsulja
* @version 1.0
*/
public class CaptchaBundle {

private final String b64Image;

private final String code;

private final String challengeId;


public CaptchaBundle(String b64Image, String code, String challengeId) {
this.b64Image = b64Image;
this.code = code;
this.challengeId = challengeId;
}

public String getB64Image() {
return b64Image;
}

public String getCode() {
return code;
}

public String getCaptchaId() {
return challengeId;
}

@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
CaptchaBundle that = (CaptchaBundle) o;
return Objects.equals(code, that.code) && Objects.equals(challengeId, that.challengeId);
}

@Override
public int hashCode() {
return Objects.hash(code, challengeId);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package edu.cornell.mannlib.vitro.webapp.beans;

public enum CaptchaDifficulty {
EASY,
HARD
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package edu.cornell.mannlib.vitro.webapp.beans;

public enum CaptchaImplementation {
RECAPTCHAV2,
NANOCAPTCHA,
NONE;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
/* $This file is distributed under the terms of the license in LICENSE$ */

package edu.cornell.mannlib.vitro.webapp.beans;
ivanmrsulja marked this conversation as resolved.
Show resolved Hide resolved

import java.io.IOException;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.concurrent.TimeUnit;

import com.google.common.base.Strings;
import com.google.common.cache.Cache;
import com.google.common.cache.CacheBuilder;
import edu.cornell.mannlib.vitro.webapp.config.ConfigurationProperties;


/**
* This class provides services related to CAPTCHA challenges and validation.
* It includes method delegates for generating challenges, validating challenge responses,
* and managing CAPTCHA challenges for specific hosts.
*
* @author Ivan Mrsulja
* @version 1.0
*/
public class CaptchaServiceBean {

private static final Cache<String, CaptchaBundle> captchaChallenges =
CacheBuilder.newBuilder()
.maximumSize(1000)
.expireAfterWrite(5, TimeUnit.MINUTES)
.build();

private static AbstractCaptchaProvider captchaProvider;

static {
CaptchaImplementation captchaImplementation = getCaptchaImpl();
switch (captchaImplementation) {
case RECAPTCHAV2:
captchaProvider = new Recaptchav2Provider();
break;
case NANOCAPTCHA:
captchaProvider = new NanocaptchaProvider();
break;
case NONE:
captchaProvider = new DummyCaptchaProvider();
break;
}
}

/**
* Generates a new CAPTCHA challenge (returns empty CaptchaBundle for 3rd party providers).
*
* @return A CaptchaBundle containing the CAPTCHA image in Base64 format, the content,
* and a unique identifier.
* @throws IOException If an error occurs during image conversion.
*/
public static CaptchaBundle generateRefreshedChallenge() throws IOException {
return captchaProvider.generateRefreshChallenge();
}

/**
* Retrieves a CAPTCHA challenge for a specific host based on the provided CAPTCHA ID
* Removes the challenge from the storage after retrieval.
*
* @param captchaId The CAPTCHA ID to match.
* @return An Optional containing the CaptchaBundle if a matching challenge is found,
* or an empty Optional otherwise.
*/
public static Optional<CaptchaBundle> getChallenge(String captchaId) {
CaptchaBundle challengeForHost = captchaChallenges.getIfPresent(captchaId);
if (challengeForHost == null) {
return Optional.empty();
}

captchaChallenges.invalidate(captchaId);

return Optional.of(challengeForHost);
}

/**
* Gets the map containing CAPTCHA challenges for different hosts.
*
* @return A ConcurrentHashMap with host addresses as keys and CaptchaBundle objects as values.
*/
public static Cache<String, CaptchaBundle> getCaptchaChallenges() {
return captchaChallenges;
}

/**
* Retrieves the configured captcha implementation based on the application's configuration properties.
* If captcha functionality is disabled, returns NONE. If the captcha implementation is not specified,
* defaults to NANOCAPTCHA.
*
* @return The selected captcha implementation (NANOCAPTCHA, RECAPTCHAv2, or NONE).
*/
public static CaptchaImplementation getCaptchaImpl() {
String captchaEnabledSetting = ConfigurationProperties.getInstance().getProperty("captcha.enabled");

if (Objects.nonNull(captchaEnabledSetting) && !Boolean.parseBoolean(captchaEnabledSetting)) {
return CaptchaImplementation.NONE;
}

String captchaImplSetting =
ConfigurationProperties.getInstance().getProperty("captcha.implementation");

if (Strings.isNullOrEmpty(captchaImplSetting) ||
(!captchaImplSetting.equalsIgnoreCase(CaptchaImplementation.RECAPTCHAV2.name()) &&
!captchaImplSetting.equalsIgnoreCase(CaptchaImplementation.NANOCAPTCHA.name()))) {
captchaImplSetting = CaptchaImplementation.NANOCAPTCHA.name();
}

return CaptchaImplementation.valueOf(captchaImplSetting.toUpperCase());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it is better to test if implementation in property file is one of supported values first, then for all other cases: missing implemenation value, misspelled value; return NANOCAPTCHA?
Otherwise it should throw IllegalArgumentException if value is misspelled which should result in broken contact form I guess.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right about misspelled values, but I think that it is more intuitive to firstly check all error logic as guard cases and then have the desired logic after that, looks more readable to me. I will refactor the code to support misspelled values and to address a suggestion below, and after that, if you still think this is unintuitive I can change it as well 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is improved to fall back to NANOCAPTCHA impl when misspelled configurations are provided.

}

/**
* Adds captcha-related fields to the given page context map. The specific fields added depend on the
* configured captcha implementation.
*
* @param context The page context map to which captcha-related fields are added.
* @throws IOException If there is an IO error during captcha challenge generation.
*/
public static void addCaptchaRelatedFieldsToPageContext(Map<String, Object> context) throws IOException {
CaptchaImplementation captchaImpl = getCaptchaImpl();
context.put("captchaToUse", captchaImpl.name());
captchaProvider.addCaptchaRelatedFieldsToPageContext(context);
}

/**
* Validates a user's captcha input.
*
* @param captchaInput The user's input for the captcha challenge.
* @param challengeId The unique identifier for the challenge (if captcha is 3rd party, this param is ignored).
* @return {@code true} if the captcha input is valid, {@code false} otherwise.
*/
public static boolean validateCaptcha(String captchaInput, String challengeId) {
return captchaProvider.validateCaptcha(captchaInput, challengeId);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package edu.cornell.mannlib.vitro.webapp.beans;

import java.util.Map;

/**
* DummyCaptchaProvider is a concrete implementation of AbstractCaptchaProvider,
* serving as a fallback when CAPTCHA is disabled. Validation will always pass,
* in order to fulfill validation logic.
*
* @see AbstractCaptchaProvider
* @see CaptchaBundle
*/
public class DummyCaptchaProvider extends AbstractCaptchaProvider {

@Override
CaptchaBundle generateRefreshChallenge() {
return new CaptchaBundle("", "", ""); // No refresh challenges if there is no implementation
}

@Override
void addCaptchaRelatedFieldsToPageContext(Map<String, Object> context) {
// No added fields necessary if there is no implementation
}

@Override
boolean validateCaptcha(String captchaInput, String challengeId) {
return true; // validation always passes
}
}
Loading
Loading