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

Buffer Overrun #24

Closed
xanather opened this issue Jan 3, 2021 · 6 comments
Closed

Buffer Overrun #24

xanather opened this issue Jan 3, 2021 · 6 comments

Comments

@xanather
Copy link

xanather commented Jan 3, 2021

Give this a try (SHA256 fingerprint):

base64_decode("lUsNVLwTUOmB9VNuaD96gWg7NnkpnqIle4+baDXCVIE", false).

Results in out-of-bounds indexing at:

if (encoded_string[pos+3] != '=' && encoded_string[pos+3] != '.') {

@Bouska
Copy link

Bouska commented Jan 4, 2021

Which version are you using? I've just checked with current master (2.rc.07) and your line of code and I don't get your error. However, I do get an std::runtime_error: Input is not valid base64-encoded data. error, because your string is not a valid base64 string. Your string is 43 characters so the '\0' of the char array is interpreted as the 44th char of the base64 string when it should not.

Try with base64_decode("lUsNVLwTUOmB9VNuaD96gWg7NnkpnqIle4+baDXCVIE=", false), it shoul work.

It might be a good idea to add a sanity check on the size of the base64 string, so an invalid string like this is catched before doing all the work for nothing.

@xanather
Copy link
Author

xanather commented Jan 4, 2021

Sorry, I was using MSVC (enable debug mode and it will catch "encoded_string[pos+3]" as being out of bounds as part of its checks).

I guess it should be documented that this library doesn't support non-padded base64. That string is a SHA256 ED25519 fingerprint given by cloud-init to /dev/console when initializing openssl server, works fine with other algorithms I've tried.

@Bouska
Copy link

Bouska commented Jan 4, 2021

I only tested with clang and GCC, I'll have to check with MSVC.

I think it should be better to add the support to non-padded data and be RFC2045-compliant (I didn't know that the padding was optional) than just document it.

@Satancito
Copy link

Satancito commented Jan 6, 2021

@ReneNyffenegger add this before string decoding, Call PadBase64(original_string)
Add some modifications if needed.

#include <iostream>

typedef std::string String;

String PadRight(String data, const size_t &totalWidth, const char &padding)
{
	if (data.length() >= totalWidth)
	{
		return data;
	}
	data.resize(totalWidth, padding);
	return data;
}

String PadBase64(String data)
{
  int modulo = data.length() % 4;
  return PadRight(data, data.length() + (modulo > 0 ? 4 - modulo : 0), u8'=');
}

int main() {
  String nonPaddedBase64 = u8"lUsNVLwTUOmB9VNuaD96gWg7NnkpnqIle4+baDXCVIE";
  std::cout<< "Non Padded :" <<nonPaddedBase64 << std::endl;
  std::cout<< "Padded     :" <<PadBase64(nonPaddedBase64) << std::endl << std::endl;

  nonPaddedBase64 = u8"QEA";
  std::cout<< "Non Padded :" <<nonPaddedBase64 << std::endl;
  std::cout<< "Padded     :" <<PadBase64(nonPaddedBase64) << std::endl << std::endl;

  nonPaddedBase64 = u8"QA";
  std::cout<< "Non Padded :" <<nonPaddedBase64 << std::endl;
  std::cout<< "Padded     :" <<PadBase64(nonPaddedBase64) << std::endl << std::endl;
}

Output

clang++-7 -pthread -std=c++17 -o main main.cpp
./main
Non Padded :lUsNVLwTUOmB9VNuaD96gWg7NnkpnqIle4+baDXCVIE
Padded     :lUsNVLwTUOmB9VNuaD96gWg7NnkpnqIle4+baDXCVIE=

Non Padded :QEA
Padded     :QEA=

Non Padded :QA
Padded     :QA==

Check this code in https://repl.it/@JomaCorpFX/Base64Padding#main.cpp

@unixwitch
Copy link

i noticed this as well while auditing the code for use in a project. you can easily reproduce the problem by replacing encoded_string[x] with encoded_string.at(x) in decode(), which will cause inputs of the wrong length (missing padding or truncated) to throw std::out_of_range.

the current implementation using operator[] will access memory outside the bounds of the input string, which is undefined behaviour and potentially a nasty security bug when processing untrusted data.

@ReneNyffenegger
Copy link
Owner

I believe this problem is fixed with V2.rc.08

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

Successfully merging a pull request may close this issue.

5 participants