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

New encryption functions #184

Merged
merged 7 commits into from Feb 11, 2018

Conversation

Projects
10 participants
@SDraw
Contributor

SDraw commented Jan 12, 2018

Based on @Elengar 's tests teaEncode works as intended, but teaDecode was pushing decoded data as null-terminated string into VM. Clearly, that's not good for decoded binary data.
Sorry, but I can't compile this for testing because of outdated version of Visual Studio.

teaDecode string pushing fix
Based on Elengar#3846's tests teaEncode works as intended, but teaDecode was pushing decoded data as null-terminated string into VM. Clearly, that's not good for decoded binary data.
Sorry, but I can't compile this for testing because of outdated version of Visual Studio.
@SDraw

This comment has been minimized.

Show comment
Hide comment
@SDraw

SDraw Jan 12, 2018

Contributor

Now I see that decoding makes data to be 4 bytes aligned. Need to find workaround for this situation.

Contributor

SDraw commented Jan 12, 2018

Now I see that decoding makes data to be 4 bytes aligned. Need to find workaround for this situation.

@Elengar

This comment has been minimized.

Show comment
Hide comment
@Elengar

Elengar Jan 12, 2018

It's because XTEA is a block cipher and block size is 4 bytes.
There are a lot of ways to pad data and then remove padding correctly, may be we need to choose one them
https://en.wikipedia.org/wiki/Padding_%28cryptography%29#Byte_padding
https://www.di-mgt.com.au/cryptopad.html

I guess that the best way is to use PKCS7 (https://en.wikipedia.org/wiki/Padding_%28cryptography%29#PKCS7)

Or even better way, (thanks stm#9360 from discord) -- just leave padding as is.
If someone need his own better padding - he can pad data himself and not allow MTA to pad zeros to the end.

Elengar commented Jan 12, 2018

It's because XTEA is a block cipher and block size is 4 bytes.
There are a lot of ways to pad data and then remove padding correctly, may be we need to choose one them
https://en.wikipedia.org/wiki/Padding_%28cryptography%29#Byte_padding
https://www.di-mgt.com.au/cryptopad.html

I guess that the best way is to use PKCS7 (https://en.wikipedia.org/wiki/Padding_%28cryptography%29#PKCS7)

Or even better way, (thanks stm#9360 from discord) -- just leave padding as is.
If someone need his own better padding - he can pad data himself and not allow MTA to pad zeros to the end.

@SDraw

This comment has been minimized.

Show comment
Hide comment
@SDraw

SDraw Jan 12, 2018

Contributor

I've thought that encoding/decoding compatibilty will be lost for data that was passed in previous MTA versions, so it's a problem.

What can be done:

  • Add padding in source code
  • Throw off the burden of padding on scripters

In both cases scripters must be informed about this change.

Contributor

SDraw commented Jan 12, 2018

I've thought that encoding/decoding compatibilty will be lost for data that was passed in previous MTA versions, so it's a problem.

What can be done:

  • Add padding in source code
  • Throw off the burden of padding on scripters

In both cases scripters must be informed about this change.

@ccw808

This comment has been minimized.

Show comment
Hide comment
@ccw808

ccw808 Jan 12, 2018

Member

Wiki example on this page shows how to handle binary data

Member

ccw808 commented Jan 12, 2018

Wiki example on this page shows how to handle binary data

@SDraw

This comment has been minimized.

Show comment
Hide comment
@SDraw

SDraw Jan 12, 2018

Contributor

Wiki example on this page shows how to handle binary data

That was the main reason to fix teaDecode in a first place. TEA/XTEA should work with any data, and problem with wrong decoding only present in MTA.
Besides, full data manipulation with wiki example is:

  • Encoding: data -> base64 (from script) -> TEA/XTEA (in source) -> base64 (in source)
  • Decoding: data -> base64 (in source) -> TEA/XTEA (in source) -> base64 (from script)

This is not good at all.

As @Necktrox has suggested it's better to keep teaDecode as it is and make separated functions for encryption/decryption, like

encodeString(string algorithm, string input, table options)
decodeString(string algorithm, string input, table options)

I agree that this way will be far better than lost compatibility.

Contributor

SDraw commented Jan 12, 2018

Wiki example on this page shows how to handle binary data

That was the main reason to fix teaDecode in a first place. TEA/XTEA should work with any data, and problem with wrong decoding only present in MTA.
Besides, full data manipulation with wiki example is:

  • Encoding: data -> base64 (from script) -> TEA/XTEA (in source) -> base64 (in source)
  • Decoding: data -> base64 (in source) -> TEA/XTEA (in source) -> base64 (from script)

This is not good at all.

As @Necktrox has suggested it's better to keep teaDecode as it is and make separated functions for encryption/decryption, like

encodeString(string algorithm, string input, table options)
decodeString(string algorithm, string input, table options)

I agree that this way will be far better than lost compatibility.

@Elengar

This comment has been minimized.

Show comment
Hide comment
@Elengar

Elengar Jan 13, 2018

@ccw808 but with using base64 it doubles file size.
Okay if we don't want to lost backward compatibility, we can use here lua_pushlstring but before pushing we can trim all trailing zero bytes, instead of using lua_pushstring which cut off all after first found zero byte. So now we have both, backward compatibility, and the possibility to use teaEncode/teaDecode with byte data directly.
And if you worried about what if someone will encrypts data that have a lot of trailing zeros and it matters - again, he can pad the data himself and not allow MTA to eat any trailing zero in the end.

Elengar commented Jan 13, 2018

@ccw808 but with using base64 it doubles file size.
Okay if we don't want to lost backward compatibility, we can use here lua_pushlstring but before pushing we can trim all trailing zero bytes, instead of using lua_pushstring which cut off all after first found zero byte. So now we have both, backward compatibility, and the possibility to use teaEncode/teaDecode with byte data directly.
And if you worried about what if someone will encrypts data that have a lot of trailing zeros and it matters - again, he can pad the data himself and not allow MTA to eat any trailing zero in the end.

@4O4

This comment has been minimized.

Show comment
Hide comment
@4O4

4O4 Jan 13, 2018

Contributor

Fixing teaEncode is good idea because of ~30% of file size overhead when using b64, but IMO native AES support would be much better for binary files anyway.

Contributor

4O4 commented Jan 13, 2018

Fixing teaEncode is good idea because of ~30% of file size overhead when using b64, but IMO native AES support would be much better for binary files anyway.

@SDraw

This comment has been minimized.

Show comment
Hide comment
@SDraw

SDraw Jan 13, 2018

Contributor

Need to decide:

  • Should function be backward compatible?
  • Should it care about padding?
  • Or leave it as it is and make new one?
Contributor

SDraw commented Jan 13, 2018

Need to decide:

  • Should function be backward compatible?
  • Should it care about padding?
  • Or leave it as it is and make new one?

@SDraw SDraw changed the title from teaDecode string pushing fix to teaEncode/Decode fix Jan 13, 2018

@Elengar

This comment has been minimized.

Show comment
Hide comment
@Elengar

Elengar Jan 13, 2018

@4O4 not 30%, full 100%, it doubles size.

Elengar commented Jan 13, 2018

@4O4 not 30%, full 100%, it doubles size.

@4O4

This comment has been minimized.

Show comment
Hide comment
@4O4

4O4 Jan 13, 2018

Contributor

@Elengar I was talking about additional size overhead introduced by the Base64 itself and it's known for a fact that it's always around 30%. teaEncode adds the rest ;)

Contributor

4O4 commented Jan 13, 2018

@Elengar I was talking about additional size overhead introduced by the Base64 itself and it's known for a fact that it's always around 30%. teaEncode adds the rest ;)

@ccw808

This comment has been minimized.

Show comment
Hide comment
@ccw808

ccw808 Jan 13, 2018

Member

I agree with @Necktrox's suggestion of new functions like encodeString/decodeString

Member

ccw808 commented Jan 13, 2018

I agree with @Necktrox's suggestion of new functions like encodeString/decodeString

SDraw added some commits Jan 14, 2018

Restored teaEncode/teaDecode
Added encryptString and decryptString with TEA algorithm, no padding

@SDraw SDraw changed the title from teaEncode/Decode fix to New encryption functions Jan 14, 2018

@Disinterpreter

This comment has been minimized.

Show comment
Hide comment
@Disinterpreter

Disinterpreter Jan 16, 2018

Anyone. Watch last PR commits

Disinterpreter commented Jan 16, 2018

Anyone. Watch last PR commits

@Necktrox

This comment has been minimized.

Show comment
Hide comment
@Necktrox

Necktrox Jan 16, 2018

Contributor

Look at the passwordHash function and use an option table (with a key member for TEA) instead of the key string.

Contributor

Necktrox commented Jan 16, 2018

Look at the passwordHash function and use an option table (with a key member for TEA) instead of the key string.

@PrototypeX

This comment has been minimized.

Show comment
Hide comment
@PrototypeX

PrototypeX Feb 3, 2018

Hello, I'm a creator one of the biggest Russian project https://smotramta.ru/ with average daily online about 1000 players. Now we have a lot of custom replaced models and I think this pull request will be very useful for us, because so we can develop our project without fear that someone can steal all our models.

Proof: https://image.ibb.co/mF0FpR/online.jpg

PrototypeX commented Feb 3, 2018

Hello, I'm a creator one of the biggest Russian project https://smotramta.ru/ with average daily online about 1000 players. Now we have a lot of custom replaced models and I think this pull request will be very useful for us, because so we can develop our project without fear that someone can steal all our models.

Proof: https://image.ibb.co/mF0FpR/online.jpg

@Disinterpreter

This comment has been minimized.

Show comment
Hide comment
@Disinterpreter

Disinterpreter Feb 4, 2018

What with Travis? It is frozen?

Disinterpreter commented Feb 4, 2018

What with Travis? It is frozen?

@qaisjp

This comment has been minimized.

Show comment
Hide comment
@qaisjp

qaisjp Feb 4, 2018

Member

Looks like it. I've restarted the job.

Member

qaisjp commented Feb 4, 2018

Looks like it. I've restarted the job.

@Necktrox

This comment has been minimized.

Show comment
Hide comment
@Necktrox

Necktrox Feb 5, 2018

Contributor

Looks fine, should be merged after a test.

Contributor

Necktrox commented Feb 5, 2018

Looks fine, should be merged after a test.

SDraw and others added some commits Feb 11, 2018

@Necktrox Necktrox merged commit c27fcaa into multitheftauto:master Feb 11, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@patrikjuvonen patrikjuvonen added this to In progress in release/v1.5.6 via automation Aug 7, 2018

@patrikjuvonen patrikjuvonen added this to the 1.5.6 milestone Aug 7, 2018

@patrikjuvonen patrikjuvonen moved this from In progress to Done in release/v1.5.6 Aug 7, 2018

@patrikjuvonen patrikjuvonen assigned ccw808, qaisjp and botder and unassigned qaisjp Sep 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment