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

Security of ValueLinkApi.java #6

Open
vraychev opened this issue Apr 4, 2018 · 3 comments
Open

Security of ValueLinkApi.java #6

vraychev opened this issue Apr 4, 2018 · 3 comments

Comments

@vraychev
Copy link

vraychev commented Apr 4, 2018

In our static observation of the code, we see some strange things in ValueLinkApi.java and DesCrypt.java. Would it be possible for the protocols to be secured?

Initialization vector are set to 0s, which is insecure:

byte[] zeros = { 0, 0, 0, 0, 0, 0, 0, 0 };
IvParameterSpec iv = new IvParameterSpec(zeros);

DES is used, which is old and insecure.

@minifreak
Copy link
Contributor

The url used for ValueLink uses already https, if that's what you mean. Check payment.properties

payment.valuelink.url=https://www.callit.com/vltest/api1.asp

Be aware that ValueLink integration may be outdated.


As for DES, you probably right. We must look into it ASAP. I'll keep you posted.

BTW, it would be great if you can split into different tickets next time. It's more manageable for us in that way.

@pplx
Copy link
Contributor

pplx commented Apr 4, 2018

Thanks for reporting this to us. Both the zero IV and DES usage are inherited from the upstream project, where it hasn't changed either, probably from lack of use. So we'll have to decide how to handle that case.

@madppiper
Copy link
Contributor

We contacted the makers of ValueLink (First Data), to see whether ValueLink is still supported. They migrated to a different product line sometime in 2013, so it is unclear whether the current API is supported at all.

It is a rarely used functionality atm, so we haven't had a chance to check into it in full yet. We will see whether the API has changed before updating the sources, since it makes sense to do it both at the same time. We will keep this ticket open, but it seems as if the overall effect on Scipio ERP is low otherwise.

pplx added a commit that referenced this issue Sep 7, 2018
Removes -kek-old command line arg from org.ofbiz.base.crypto.Main. Fixes
one or two warnings.

DES is insecure and the remaining DES code in Scipio, should not be used
to encrypt new records only to decrypt existing ones.

Since ValueLink is not working anymore anyway, there is no reason to
leave this in.

For emergency testing (ONLY), the code is left commented in Main.java
and could be uncommented.

Refs https://gitlab.ilscipio.com/scipio-dev/dev/scipioce-dev/issues/489,
https://gitlab.ilscipio.com/scipio-dev/dev/scipioce-dev/issues/488,
#6
pplx added a commit that referenced this issue Sep 14, 2018
…omments)

* String.getBytes() calls in EntityCrypto will NOT be changed to UTF-8
for time being, due to (small) risk of breaking old existing Windows
clients
* Add deprecations to dangerous methods in HashCrypt
* Remove DES-related deprecation EntityCrypto that was actually hiding
good warnings and is not really deprecated itself.

Refs https://gitlab.ilscipio.com/scipio-dev/dev/scipioce-dev/issues/659,
https://gitlab.ilscipio.com/scipio-dev/dev/scipioce-dev/issues/489,
#6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants