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 importer vulnerability #4240

Merged
merged 6 commits into from
Jul 30, 2018
Merged

Conversation

nicksw
Copy link
Contributor

@nicksw nicksw commented Jul 29, 2018

Fixed issue where importer was vulnerable to XXE attacks by
disabling DTDs. If feature is unavailable, warning is sent to logger and returns builderFactory at current state. Used #4229


  • Change in CHANGELOG.md described
  • Tests created for changes
  • Manually tested changed features in running JabRef
  • Screenshots added in PR description (for bigger UI changes)
  • Ensured that the git commit message is a good one
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)

nicksw and others added 2 commits July 29, 2018 11:05
Fixed issue JabRef#4229  where importer was vulnerable to XXE attacks by
disabling DTDs along with adding warning to logger if features are
unavailable. fixes JabRef#4229
@Siedlerchr
Copy link
Member

Thanks for your contribution
I just resolved the conflict in the changelog, so that the build runs on Travis. Check the Continous Interation Travis output (just click on it, in case it fails)

/**
* Importer for the MS Office 2007 XML bibliography format
* By S. M. Mahbub Murshed
* By S. M. Mahbub Murshed & Nicholas S. Weatherley
Copy link
Member

Choose a reason for hiding this comment

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

Pleas remove this author comment completely as we handle author credits via our authors-file

private DocumentBuilderFactory makeSafeDocBuilderFactory(DocumentBuilderFactory dBuild) {
String FEATURE = null;
try {
FEATURE = "http://apache.org/xml/features/disallow-doctype-decl";
Copy link
Member

Choose a reason for hiding this comment

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

Just some minor improvemtns: I would suggest extracting both urls to two separate variables and make them private static final as constants.

dBuild.setExpandEntityReferences(false);

} catch (ParserConfigurationException e) {
LOGGER.warn("Builder not fully configured. ParserConfigurationException was thrown. Feature:'" +
Copy link
Member

Choose a reason for hiding this comment

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

slf4j has the nice advantage that it supports variables via curly braces:
https://www.slf4j.org/faq.html#logging_performance

Copy link
Member

Choose a reason for hiding this comment

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

and please add the exception as a parameter so that the root cause is also logged. (then probably the part "ParserConfigurationException was thrown." is no longer necessary).

Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

Thanks again for your contribtution, just some minor code improvements 👍

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jul 29, 2018
Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! The code looks good but I join the nitpicking party. Please fix these minor things and then we can merge your PR.

CHANGELOG.md Outdated
@@ -60,7 +60,8 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
- We fixed an issue where the "Convert to BibTeX-Cleanup" moved the content of the `file` field to the `pdf` field [#4120](https://github.com/JabRef/jabref/issues/4120)
- We fixed an issue where the preview pane in entry preview in preferences wasn't showing the citation style selected [#3849](https://github.com/JabRef/jabref/issues/3849)
- We fixed an issue where the default entry preview style still contained the field `review`. The field `review` in the style is now replaced with comment to be consistent with the entry editor [#4098](https://github.com/JabRef/jabref/issues/4098)
- We fixed an issue where filles added via the "Attach file" contextmenu of an entry were not made relative. [#4201](https://github.com/JabRef/jabref/issues/4201)
- We an issue where users were vulnerable to XXE attacks during parsing [#4229](https://github.com/JabRef/jabref/issues/4229)
Copy link
Member

Choose a reason for hiding this comment

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

We fixed an...

* @return If supported, XXE safe DocumentBuilderFactory. Else, returns original builder given
*/
private DocumentBuilderFactory makeSafeDocBuilderFactory(DocumentBuilderFactory dBuild) {
String FEATURE = null;
Copy link
Member

Choose a reason for hiding this comment

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

We have the convention that only static (constant) values are all capitals. Thus, please change the name to feature.

dBuild.setExpandEntityReferences(false);

} catch (ParserConfigurationException e) {
LOGGER.warn("Builder not fully configured. ParserConfigurationException was thrown. Feature:'" +
Copy link
Member

Choose a reason for hiding this comment

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

and please add the exception as a parameter so that the root cause is also logged. (then probably the part "ParserConfigurationException was thrown." is no longer necessary).

Removed author line in class comment. Reworded CHANGLOG.md. Set
DTD features to individual final static constants. Optimized
logger by parameterizing feature and error.
@nicksw
Copy link
Contributor Author

nicksw commented Jul 30, 2018

Thanks for the feedback. Yes, I agree with all of the fixes. Sorry about the two commits back to back. The first one stated it did not go through on my end as I needed to update HEAD. Also, I am not sure why I am failing both tests. the CI test seems to be related to GUI errors while the Codacy fail is from the extra space before importing items from slf4j.

@Siedlerchr
Copy link
Member

The Travis CI log raises a check style issue, so just some code formatting and the wrong import order. Although it might seem annoying, but it serves the purpose to have consistent code style and to avoid unnecessary conflicts between files

Both for intellij and Eclipse we provide the correct code format layout. In Eclipse hit ctrl + f to format and ctrl + o for the import order fix

@nicksw
Copy link
Contributor Author

nicksw commented Jul 30, 2018

No I agree. The project should be uniform in import and code style as it would look a mess without. Pushed the changes.

Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

Thanks again for your contribution!

@tobiasdiez tobiasdiez merged commit 89f855d into JabRef:master Jul 30, 2018
@nicksw nicksw deleted the fix-for-issue-4229 branch August 4, 2018 12:35
Siedlerchr added a commit that referenced this pull request Aug 8, 2018
…tive

* upstream/master:
  Update dependencies (#4251)
  Fix author list parser (#4169) (#4248)
  Solved #3823 File annotation (#4246)
  Fix importer vulnerability (#4240)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants