Fix/captcha repeater vulnerability #427

ivanmrsulja · 2023-11-22T13:29:26Z

VIVO GitHub issue: Mitigate vulnerability of Captcha feature
VIVO PR

What does this pull request do?

This PR mitigates vulnerability of captcha feature by introducing new methods of challenge generation: nanocaptcha and Google reCAPTCHA.

What's new?

A CaptrchaServiceBean is created which can both provide and validate text-based challenges as well as provide validation services for Google reCaptcha through Google API. I also included a configuration for which captcha method is used so you can use both methods interchangeably (just change configuration options).

How should this be tested?

Unit tests for CaptchaServiceBean are created. If you want to test manually, simply run the application and follow instructions on how to configure each captcha method (instructions are available in example.runtime.properties file).
Captcha is currently only used on /contact page. Keep in mind that you have to setup SMTP properties as well as default contact email for feedback form to be accessible.

Interested parties

@litvinovg @chenejac @brianjlowe

chenejac

@ivanmrsulja thanks for this contribution. Please check one my comment.

api/pom.xml

home/src/main/resources/rdf/i18n/de_DE/interface-i18n/firsttime/vitro_UiLabel.ttl

home/src/main/resources/rdf/i18n/pt_BR/interface-i18n/firsttime/vitro_UiLabel.ttl

litvinovg

Thanks for the PR!
Please take a look at some comments I did and translation suggestions.
I tested self hosted captcha. It works well.
Recaptcha V2 works.
Recaptcha V3 doesn't work (image below)
I didn't found any notes on what Recaptcha version should be supported.
I think it should be documented or V3 fixed if that was expected to be working.

api/src/main/java/edu/cornell/mannlib/vitro/webapp/beans/CaptchaServiceBean.java

.../main/java/edu/cornell/mannlib/vitro/webapp/controller/freemarker/ContactFormController.java

.../main/java/edu/cornell/mannlib/vitro/webapp/controller/freemarker/ContactMailController.java

home/src/main/resources/rdf/i18n/ru_RU/interface-i18n/firsttime/vitro_UiLabel.ttl

.../main/java/edu/cornell/mannlib/vitro/webapp/controller/freemarker/ContactMailController.java

api/src/main/java/edu/cornell/mannlib/vitro/webapp/beans/CaptchaServiceBean.java

ivanmrsulja · 2023-12-11T15:58:12Z

Thanks for the PR! Please take a look at some comments I did and translation suggestions. I tested self hosted captcha. It works well. Recaptcha V2 works. Recaptcha V3 doesn't work (image below) I didn't found any notes on what Recaptcha version should be supported. I think it should be documented or V3 fixed if that was expected to be working.

Yes, in the requirements, it was stated that there should be an "I'm not a robot field" which is reCAPTCHA v2 implementation. V3 runs in the background and it is a pure JS API that returns a score (based on number and speed of the interactions with the webpage). That score is the indicator whether additional measures should be taken for each interaction, it is a supplementary library. I did not plan to encorporate this functionality but it is open for discussion on the next meeting.

…lidation logic.

…e/vitro_UiLabel.ttl Co-authored-by: Benjamin Kampe <[email protected]>

…e/vitro_UiLabel.ttl Co-authored-by: salmjunior <[email protected]>

…here unnecessary.

…e/vitro_UiLabel.ttl Co-authored-by: Georgy Litvinov <[email protected]>

litvinovg

Please update comment in example.runtime.properties to mention that only re-captcha v2 keys are supported in this PR and VIVO PR as was discussed at the meeting on Tuesday.
I suggest to convert string values of captcha.implementation in Java code to static fields.
Also, the name for self hosted captcha can be better I think.
Maybe we could use implementation name, like nanocaptcha in case somebody in future decides to add another self hosted captcha implementation. Not sure it should be in upper case, lower case looks better to me.
@chenejac What is your opinion?

chenejac

@ivanmrsulja I have added a comment which reflects committers' meeting discussion. Please, take a look.

home/src/main/resources/config/example.runtime.properties

litvinovg

Please convert captcha implementation options to constants or enumerations throughout the PR. A few more comments in the code.

home/src/main/resources/config/example.runtime.properties

api/src/main/java/edu/cornell/mannlib/vitro/webapp/beans/CaptchaServiceBean.java

webapp/src/main/webapp/templates/freemarker/body/contactForm/contactForm-form.ftl

home/src/main/resources/rdf/i18n/fr_CA/interface-i18n/firsttime/vitro_UiLabel.ttl

litvinovg · 2023-12-21T13:48:27Z

api/src/main/java/edu/cornell/mannlib/vitro/webapp/beans/CaptchaServiceBean.java

+            captchaImplSetting = "NANOCAPTCHA";
+        }
+
+        return CaptchaImplementation.valueOf(captchaImplSetting.toUpperCase());


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.

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 😄

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

litvinovg · 2023-12-21T14:24:59Z

api/src/main/java/edu/cornell/mannlib/vitro/webapp/beans/CaptchaServiceBean.java

+        switch (captchaImpl) {
+            case RECAPTCHAV2:
+                if (CaptchaServiceBean.validateReCaptcha(captchaInput)) {
+                    return true;
+                }
+                break;
+            case NANOCAPTCHA:
+                Optional<CaptchaBundle> optionalChallenge = CaptchaServiceBean.getChallenge(challengeId);
+                if (optionalChallenge.isPresent() && optionalChallenge.get().getCode().equals(captchaInput)) {
+                    return true;
+                }
+                break;
+            case NONE:
+                return true;
+        }


What if another captcha implementation is added?
Should there be a class for each captcha implementation?
Now class responsibilities seem a little bit mixed.

I can make a provider class for each implementation

I have refactored code and made providers for each implementation. ServiceBean only delegates requests to providers and is responsible for challenge persistence (in-memory).

litvinovg

Default difficulty should be easy.
I tested nanocaptcha and recaptchav2, works fine.

home/src/main/resources/config/example.runtime.properties

Co-authored-by: Georgy Litvinov <[email protected]>

chenejac

@ivanmrsulja well done

ivanmrsulja mentioned this pull request Nov 22, 2023

Added captcha configuration example. vivo-project/VIVO#3920

Merged

chenejac requested a review from litvinovg November 22, 2023 15:01

chenejac linked an issue Nov 22, 2023 that may be closed by this pull request

Mitigate vulnerability of Captcha feature vivo-project/VIVO#3918

Closed

chenejac requested changes Nov 22, 2023

View reviewed changes

api/pom.xml Outdated Show resolved Hide resolved

ivanmrsulja requested a review from chenejac November 23, 2023 08:41

bkampe suggested changes Dec 5, 2023

View reviewed changes

home/src/main/resources/rdf/i18n/de_DE/interface-i18n/firsttime/vitro_UiLabel.ttl Outdated Show resolved Hide resolved

salmjunior suggested changes Dec 5, 2023

View reviewed changes

home/src/main/resources/rdf/i18n/pt_BR/interface-i18n/firsttime/vitro_UiLabel.ttl Outdated Show resolved Hide resolved

home/src/main/resources/rdf/i18n/pt_BR/interface-i18n/firsttime/vitro_UiLabel.ttl Outdated Show resolved Hide resolved

ivanmrsulja requested review from bkampe and salmjunior December 5, 2023 10:48

litvinovg requested changes Dec 11, 2023

View reviewed changes

ivanmrsulja and others added 18 commits December 11, 2023 17:37

Added captcha generation using nano captcha library. Added initial va…

a503bb4

…lidation logic.

Added variable challenge length.

461213d

Added Google reCaptcha. Added simple configuration feature toggle.

d06b400

Added java docs.

58bdd92

Fixed log4J version bug. Added unit tests for captcha functionality.

ca18759

Added guava cache for mitigation of DoS by generating challenges.

fe87586

Aligned sl4j-api dependency version.

262b99a

Added refresh button for default challenge.

83c553b

Improved error messages and added localization.

9156eba

Update home/src/main/resources/rdf/i18n/de_DE/interface-i18n/firsttim…

0655072

…e/vitro_UiLabel.ttl Co-authored-by: Benjamin Kampe <[email protected]>

Update home/src/main/resources/rdf/i18n/pt_BR/interface-i18n/firsttim…

c5d6573

…e/vitro_UiLabel.ttl Co-authored-by: salmjunior <[email protected]>

Update home/src/main/resources/rdf/i18n/pt_BR/interface-i18n/firsttim…

d616c95

…e/vitro_UiLabel.ttl Co-authored-by: salmjunior <[email protected]>

Added missing licence header. Refactored code to avoid passing vreq w…

3f86241

…here unnecessary.

Update home/src/main/resources/rdf/i18n/ru_RU/interface-i18n/firsttim…

b9d7620

…e/vitro_UiLabel.ttl Co-authored-by: Georgy Litvinov <[email protected]>

Update home/src/main/resources/rdf/i18n/ru_RU/interface-i18n/firsttim…

3060975

…e/vitro_UiLabel.ttl Co-authored-by: Georgy Litvinov <[email protected]>

Update home/src/main/resources/rdf/i18n/ru_RU/interface-i18n/firsttim…

bdc9820

…e/vitro_UiLabel.ttl Co-authored-by: Georgy Litvinov <[email protected]>

Update home/src/main/resources/rdf/i18n/ru_RU/interface-i18n/firsttim…

187dc85

…e/vitro_UiLabel.ttl Co-authored-by: Georgy Litvinov <[email protected]>

Switched to using getInstance() instead of deprecated getBean() method.

c195f60

ivanmrsulja force-pushed the fix/captcha-repeater-vulnerability branch from 62fffeb to c195f60 Compare December 12, 2023 15:50

Fixed frontend display error.

4233deb

ivanmrsulja requested a review from litvinovg December 13, 2023 12:31

litvinovg requested changes Dec 13, 2023

View reviewed changes

chenejac previously approved these changes Dec 15, 2023

View reviewed changes

home/src/main/resources/config/example.runtime.properties Outdated Show resolved Hide resolved

Added captcha feature toggle with difficulty setting.

34842c7

ivanmrsulja dismissed chenejac’s stale review via 34842c7 December 15, 2023 15:17

Updated captcha configuration.

358e8ae

ivanmrsulja requested review from litvinovg and chenejac December 15, 2023 15:25

ivanmrsulja added 2 commits December 15, 2023 16:26

Made nanocaptcha challenge little bigger.

2e7b041

Improved spanish localization.

dee609f

litvinovg requested changes Dec 18, 2023

View reviewed changes

Made configuration options as enumertions.

71dbdfd

chenejac requested changes Dec 19, 2023

View reviewed changes

webapp/src/main/webapp/templates/freemarker/body/contactForm/contactForm-form.ftl Outdated Show resolved Hide resolved

Made configuration case insensitive, updated configuration docs.

0aa5ff7

ivanmrsulja requested review from chenejac and litvinovg December 19, 2023 11:41

michel-heon reviewed Dec 19, 2023

View reviewed changes

home/src/main/resources/rdf/i18n/fr_CA/interface-i18n/firsttime/vitro_UiLabel.ttl Outdated Show resolved Hide resolved

Improved french localization.

af1ddae

ivanmrsulja requested a review from michel-heon December 20, 2023 22:25

litvinovg requested changes Dec 21, 2023

View reviewed changes

Refactored code to use provider pattern.

aef5f86

ivanmrsulja requested a review from litvinovg December 22, 2023 11:21

Updated javadocs.

74c20c1

litvinovg reviewed Jan 9, 2024

View reviewed changes

home/src/main/resources/config/example.runtime.properties Outdated Show resolved Hide resolved

Update home/src/main/resources/config/example.runtime.properties

fc92fc3

Co-authored-by: Georgy Litvinov <[email protected]>

litvinovg self-requested a review January 9, 2024 15:10

litvinovg approved these changes Jan 9, 2024

View reviewed changes

chenejac approved these changes Jan 10, 2024

View reviewed changes

chenejac merged commit 2df1164 into vivo-project:main Jan 10, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/captcha repeater vulnerability #427

Fix/captcha repeater vulnerability #427

ivanmrsulja commented Nov 22, 2023 •

edited by litvinovg

Loading

chenejac left a comment

litvinovg left a comment

ivanmrsulja commented Dec 11, 2023

litvinovg left a comment •

edited

Loading

chenejac left a comment

litvinovg left a comment

litvinovg Dec 21, 2023

ivanmrsulja Dec 22, 2023

ivanmrsulja Dec 22, 2023

litvinovg Dec 21, 2023

ivanmrsulja Dec 22, 2023

ivanmrsulja Dec 22, 2023

litvinovg left a comment

chenejac left a comment

Fix/captcha repeater vulnerability #427

Fix/captcha repeater vulnerability #427

Conversation

ivanmrsulja commented Nov 22, 2023 • edited by litvinovg Loading

What does this pull request do?

What's new?

How should this be tested?

Interested parties

chenejac left a comment

Choose a reason for hiding this comment

litvinovg left a comment

Choose a reason for hiding this comment

ivanmrsulja commented Dec 11, 2023

litvinovg left a comment • edited Loading

Choose a reason for hiding this comment

chenejac left a comment

Choose a reason for hiding this comment

litvinovg left a comment

Choose a reason for hiding this comment

litvinovg Dec 21, 2023

Choose a reason for hiding this comment

ivanmrsulja Dec 22, 2023

Choose a reason for hiding this comment

ivanmrsulja Dec 22, 2023

Choose a reason for hiding this comment

litvinovg Dec 21, 2023

Choose a reason for hiding this comment

ivanmrsulja Dec 22, 2023

Choose a reason for hiding this comment

ivanmrsulja Dec 22, 2023

Choose a reason for hiding this comment

litvinovg left a comment

Choose a reason for hiding this comment

chenejac left a comment

Choose a reason for hiding this comment

ivanmrsulja commented Nov 22, 2023 •

edited by litvinovg

Loading

litvinovg left a comment •

edited

Loading